You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jo...@apache.org on 2019/08/06 15:41:22 UTC
svn commit: r1864526 - /httpd/httpd/trunk/modules/metadata/mod_remoteip.c
Author: jorton
Date: Tue Aug 6 15:41:22 2019
New Revision: 1864526
URL: http://svn.apache.org/viewvc?rev=1864526&view=rev
Log:
* modules/metadata/mod_remoteip.c (remoteip_process_v2_header,
remoteip_input_filter): Add sanity checks.
Submitted by: jorton, Daniel McCarney <cpu letsencrypt.org>
Modified:
httpd/httpd/trunk/modules/metadata/mod_remoteip.c
Modified: httpd/httpd/trunk/modules/metadata/mod_remoteip.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_remoteip.c?rev=1864526&r1=1864525&r2=1864526&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
+++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Tue Aug 6 15:41:22 2019
@@ -987,15 +987,13 @@ static remoteip_parse_status_t remoteip_
return HDR_ERROR;
#endif
default:
- /* unsupported protocol, keep local connection address */
- return HDR_DONE;
+ /* unsupported protocol */
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(10183)
+ "RemoteIPProxyProtocol: unsupported protocol %.2hx",
+ (unsigned short)hdr->v2.fam);
+ return HDR_ERROR;
}
break; /* we got a sockaddr now */
-
- case 0x00: /* LOCAL command */
- /* keep local connection address for LOCAL */
- return HDR_DONE;
-
default:
/* not a supported command */
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03507)
@@ -1087,11 +1085,24 @@ static apr_status_t remoteip_input_filte
/* try to read a header's worth of data */
while (!ctx->done) {
if (APR_BRIGADE_EMPTY(ctx->bb)) {
- ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
- ctx->need - ctx->rcvd);
+ apr_off_t got, want = ctx->need - ctx->rcvd;
+
+ ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block, want);
if (ret != APR_SUCCESS) {
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, f->c, APLOGNO(10184)
+ "failed reading input");
return ret;
}
+
+ ret = apr_brigade_length(ctx->bb, 1, &got);
+ if (ret || got > want) {
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, f->c, APLOGNO(10185)
+ "RemoteIPProxyProtocol header too long, "
+ "got %" APR_OFF_T_FMT " expected %" APR_OFF_T_FMT,
+ got, want);
+ f->c->aborted = 1;
+ return APR_ECONNABORTED;
+ }
}
if (APR_BRIGADE_EMPTY(ctx->bb)) {
return block == APR_NONBLOCK_READ ? APR_SUCCESS : APR_EOF;
@@ -1139,6 +1150,13 @@ static apr_status_t remoteip_input_filte
if (ctx->rcvd >= MIN_V2_HDR_LEN) {
ctx->need = MIN_V2_HDR_LEN +
remoteip_get_v2_len((proxy_header *) ctx->header);
+ if (ctx->need > sizeof(proxy_v2)) {
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(10186)
+ "RemoteIPProxyProtocol protocol header length too long");
+ f->c->aborted = 1;
+ apr_brigade_destroy(ctx->bb);
+ return APR_ECONNABORTED;
+ }
}
if (ctx->rcvd >= ctx->need) {
psts = remoteip_process_v2_header(f->c, conn_conf,
Re: svn commit: r1864526 -
/httpd/httpd/trunk/modules/metadata/mod_remoteip.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/28/2019 04:20 PM, Joe Orton wrote:
> On Wed, Aug 28, 2019 at 02:24:40PM +0200, Ruediger Pluem wrote:
>> On 08/06/2019 05:41 PM, jorton@apache.org wrote:
>>> Author: jorton
>>> Date: Tue Aug 6 15:41:22 2019
>>> New Revision: 1864526
> ...
>>> + ret = apr_brigade_length(ctx->bb, 1, &got);
>>> + if (ret || got > want) {
>>> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, f->c, APLOGNO(10185)
>>> + "RemoteIPProxyProtocol header too long, "
>>> + "got %" APR_OFF_T_FMT " expected %" APR_OFF_T_FMT,
>>> + got, want);
>>> + f->c->aborted = 1;
>>
>> Shouldn't we do apr_brigade_destroy(ctx->bb) here as well like below?
>
> The apr_brigade_destroy() calls should be all redundant in the failure
> cases AFAICT. Can you see a reason why they need to be explicitly
> destroyed prior to the pool cleanups running?
>
Not really. I just noticed that we handle it differently. So I guess just removing it in the second case would also
serve the purpose of doing the same thing in case we fail.
Regards
Rüdiger
Re: svn commit: r1864526 -
/httpd/httpd/trunk/modules/metadata/mod_remoteip.c
Posted by Joe Orton <jo...@redhat.com>.
On Wed, Aug 28, 2019 at 02:24:40PM +0200, Ruediger Pluem wrote:
> On 08/06/2019 05:41 PM, jorton@apache.org wrote:
> > Author: jorton
> > Date: Tue Aug 6 15:41:22 2019
> > New Revision: 1864526
...
> > + ret = apr_brigade_length(ctx->bb, 1, &got);
> > + if (ret || got > want) {
> > + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, f->c, APLOGNO(10185)
> > + "RemoteIPProxyProtocol header too long, "
> > + "got %" APR_OFF_T_FMT " expected %" APR_OFF_T_FMT,
> > + got, want);
> > + f->c->aborted = 1;
>
> Shouldn't we do apr_brigade_destroy(ctx->bb) here as well like below?
The apr_brigade_destroy() calls should be all redundant in the failure
cases AFAICT. Can you see a reason why they need to be explicitly
destroyed prior to the pool cleanups running?
Re: svn commit: r1864526 -
/httpd/httpd/trunk/modules/metadata/mod_remoteip.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/06/2019 05:41 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Tue Aug 6 15:41:22 2019
> New Revision: 1864526
>
> URL: http://svn.apache.org/viewvc?rev=1864526&view=rev
> Log:
> * modules/metadata/mod_remoteip.c (remoteip_process_v2_header,
> remoteip_input_filter): Add sanity checks.
>
> Submitted by: jorton, Daniel McCarney <cpu letsencrypt.org>
>
> Modified:
> httpd/httpd/trunk/modules/metadata/mod_remoteip.c
>
> Modified: httpd/httpd/trunk/modules/metadata/mod_remoteip.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/metadata/mod_remoteip.c?rev=1864526&r1=1864525&r2=1864526&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/metadata/mod_remoteip.c (original)
> +++ httpd/httpd/trunk/modules/metadata/mod_remoteip.c Tue Aug 6 15:41:22 2019
> @@ -987,15 +987,13 @@ static remoteip_parse_status_t remoteip_
> return HDR_ERROR;
> #endif
> default:
> - /* unsupported protocol, keep local connection address */
> - return HDR_DONE;
> + /* unsupported protocol */
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(10183)
> + "RemoteIPProxyProtocol: unsupported protocol %.2hx",
> + (unsigned short)hdr->v2.fam);
> + return HDR_ERROR;
> }
> break; /* we got a sockaddr now */
> -
> - case 0x00: /* LOCAL command */
> - /* keep local connection address for LOCAL */
> - return HDR_DONE;
> -
> default:
> /* not a supported command */
> ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c, APLOGNO(03507)
> @@ -1087,11 +1085,24 @@ static apr_status_t remoteip_input_filte
> /* try to read a header's worth of data */
> while (!ctx->done) {
> if (APR_BRIGADE_EMPTY(ctx->bb)) {
> - ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block,
> - ctx->need - ctx->rcvd);
> + apr_off_t got, want = ctx->need - ctx->rcvd;
> +
> + ret = ap_get_brigade(f->next, ctx->bb, ctx->mode, block, want);
> if (ret != APR_SUCCESS) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, f->c, APLOGNO(10184)
> + "failed reading input");
> return ret;
> }
> +
> + ret = apr_brigade_length(ctx->bb, 1, &got);
> + if (ret || got > want) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, ret, f->c, APLOGNO(10185)
> + "RemoteIPProxyProtocol header too long, "
> + "got %" APR_OFF_T_FMT " expected %" APR_OFF_T_FMT,
> + got, want);
> + f->c->aborted = 1;
Shouldn't we do apr_brigade_destroy(ctx->bb) here as well like below?
> + return APR_ECONNABORTED;
> + }
> }
> if (APR_BRIGADE_EMPTY(ctx->bb)) {
> return block == APR_NONBLOCK_READ ? APR_SUCCESS : APR_EOF;
> @@ -1139,6 +1150,13 @@ static apr_status_t remoteip_input_filte
> if (ctx->rcvd >= MIN_V2_HDR_LEN) {
> ctx->need = MIN_V2_HDR_LEN +
> remoteip_get_v2_len((proxy_header *) ctx->header);
> + if (ctx->need > sizeof(proxy_v2)) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f->c, APLOGNO(10186)
> + "RemoteIPProxyProtocol protocol header length too long");
> + f->c->aborted = 1;
> + apr_brigade_destroy(ctx->bb);
> + return APR_ECONNABORTED;
> + }
> }
> if (ctx->rcvd >= ctx->need) {
> psts = remoteip_process_v2_header(f->c, conn_conf,
>
>
>
Regards
Rüdiger