On Wed, Dec 7, 2016 at 2:11 PM, Ruediger Pluem <rpl...@apache.org> wrote:
> Sorry for chiming in so late :-(. Again, no worries... On 12/05/2016 03:34 PM, j...@apache.org wrote: > > Modified: httpd/httpd/branches/2.4.x/server/protocol.c > > URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/ > server/protocol.c?rev=1772678&r1=1772677&r2=1772678&view=diff > > ============================================================ > ================== > > --- httpd/httpd/branches/2.4.x/server/protocol.c (original) > > +++ httpd/httpd/branches/2.4.x/server/protocol.c Mon Dec 5 14:34:29 > 2016 > > > @@ -617,58 +646,269 @@ static int read_request_line(request_rec > > [...] > > + > > + /* Advance protocol pointer over leading whitespace, NUL terminate > the uri > > + * If non-SP whitespace is encountered, mark as specific error > > + */ > > + for (r->protocol = ll; apr_isspace(*r->protocol); ++r->protocol) > > + if (*r->protocol != ' ' && deferred_error == rrl_none) > > + deferred_error = rrl_badwhitespace; > > + *ll = '\0'; > > + > > + /* Scan the protocol up to the next whitespace, validation comes > later */ > > + if (!(ll = (char*) ap_scan_vchar_obstext(r->protocol))) { > > + len = strlen(r->protocol); > > + goto rrl_done; > > + } > > + len = ll - r->protocol; > > + > > + /* Advance over trailing whitespace, if found mark in error, > > + * determine if trailing text is found, unconditionally mark in > error, > > + * finally NUL terminate the protocol string > > + */ > > + if (*ll && !apr_isspace(*ll)) { > > + deferred_error = rrl_badprotocol; > > + } > > + else if (strict && *ll) { > > + deferred_error = rrl_excesswhitespace; > > } > > + else { > > + for ( ; apr_isspace(*ll); ++ll) > > + if (*ll != ' ' && deferred_error == rrl_none) > > + deferred_error = rrl_badwhitespace; > > + if (*ll && deferred_error == rrl_none) > > + deferred_error = rrl_trailingtext; > > + } > > + *((char *)r->protocol + len) = '\0'; > > + > > +rrl_done: > > + /* For internal integrety and palloc efficiency, reconstruct > the_request > > + * in one palloc, using only single SP characters, per spec. > > + */ > > + r->the_request = apr_pstrcat(r->pool, r->method, *uri ? " " : NULL, > uri, > > + *r->protocol ? " " : NULL, > r->protocol, NULL); > > > > - /* Provide quick information about the request method as soon as > known */ > > + if (len == 8 > > + && r->protocol[0] == 'H' && r->protocol[1] == 'T' > > + && r->protocol[2] == 'T' && r->protocol[3] == 'P' > > + && r->protocol[4] == '/' && apr_isdigit(r->protocol[5]) > > + && r->protocol[6] == '.' && apr_isdigit(r->protocol[7]) > > + && r->protocol[5] != '0') { > > + r->assbackwards = 0; > > + r->proto_num = HTTP_VERSION(r->protocol[5] - '0', > r->protocol[7] - '0'); > > + } > > + else if (len == 8 > > + && (r->protocol[0] == 'H' || r->protocol[0] == 'h') > > + && (r->protocol[1] == 'T' || r->protocol[1] == 't') > > + && (r->protocol[2] == 'T' || r->protocol[2] == 't') > > + && (r->protocol[3] == 'P' || r->protocol[3] == 'p') > > + && r->protocol[4] == '/' && apr_isdigit(r->protocol[5]) > > + && r->protocol[6] == '.' && apr_isdigit(r->protocol[7]) > > + && r->protocol[5] != '0') { > > + r->assbackwards = 0; > > + r->proto_num = HTTP_VERSION(r->protocol[5] - '0', > r->protocol[7] - '0'); > > + if (strict && deferred_error == rrl_none) > > + deferred_error = rrl_badprotocol; > > + else > > + memcpy((char*)r->protocol, "HTTP", 4); > > + } > > + else if (r->protocol[0]) { > > + r->assbackwards = 0; > > + r->proto_num = HTTP_VERSION(1,0); > > + /* Defer setting the r->protocol string till error msg is > composed */ > > + if (strict && deferred_error == rrl_none) > > + deferred_error = rrl_badprotocol; > > + else > > + r->protocol = "HTTP/1.0"; > > Hm. r->protocol is not const char*. Shouldn't we use > > apr_pstrdup(r->pool, "HTTP/1.0"); > > > + } > > + else { > > + r->assbackwards = 1; > > + r->protocol = "HTTP/0.9"; > > + r->proto_num = HTTP_VERSION(0, 9); > > + } > Does it make more sense to to apply the apr_pstrdup() against r->protocol just here, after all but one assignment is made? The same question occurs here with 0.9, and down at rrl_failed: below. We can apr_pstrdup each of these three, letting the caller manipulate the original we scanned in the successful case, or make a copy in all cases right here, once again below for 1.0. I had expected to see some caution and mentioned it in the initial commit log, but my maintainer-mode build did not trip on this. > > + else if (deferred_error == rrl_badprotocol) > > + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02418) > > + "HTTP Request Line; Unrecognized protocol > '%.*s' " > > + "(perhaps whitespace was injected?)", > > + field_name_len(r->protocol), r->protocol); > Don't we miss a > > r->protocol = apr_pstrdup(r->pool, "HTTP/1.0"); > > in this case? No, we do not, it was assigned to HTTP/0.9 above in the case of no protocol, and rrl_failed will whack it later into HTTP/1.0 to ensure we see an error (we wouldn't see a response line in HTTP/0.9 mode). > > + r->status = HTTP_BAD_REQUEST; > > + goto rrl_failed; > > + } >
