You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/05/24 23:33:40 UTC
[GitHub] [trafficserver] sudheerv opened a new pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
sudheerv opened a new pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875
This change restricts supported HTTP versions to 1.0, 1.1 and 2.0
to prevent potential mishandling, request smugging or other vulns
due to random/arbitrary version tags
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#discussion_r638362047
##########
File path: proxy/hdrs/HTTP.cc
##########
@@ -625,7 +625,27 @@ http_hdr_type_set(HTTPHdrImpl *hh, HTTPType type)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
-void
+bool
+is_version_supported(const uint8_t major, const uint8_t minor)
Review comment:
Can you clarify what "supported" means? Having a (doxygen) comment for this function would be helpful.
Do you think we should not have 3(.0) here yet?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#issuecomment-847462230
I don't have a strong opinion on whether we allow the invalid request line. However, I think the restriction should be either a) accept all the versions (i.e. `1.1`, `1.0`, `2.0` and `3.0`) that are on the RFC you quoted && can be handled by ATS , or b) accept only `1.1` and `1.0` that are the only valid versions in reality.
Option a) could be complicated. While ATS has experimental support for H3, it is only available if ATS is built with a SSL library that supports QUIC (special OpenSSL or BoringSSL). We may want to have `ifdef` in the function if we take option a). This is why I wanted you to clarify what you mean by "supported". Strictly speaking, we should check whether ALPN (or NPN) is available to say `2.0` is supported in that sense.
My biggest question was why the function doesn't allow 3.0 where it allows 2.0, but now I'm inclined to option b) because of the complexity above. I don't see benefits to have the complicated function at the moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bryancall commented on pull request #7875: Restrict HTTP versions allowed on the HTTP request line
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#issuecomment-892183544
Cherry picked to 9.1.x
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#discussion_r638363455
##########
File path: proxy/hdrs/HTTP.cc
##########
@@ -625,7 +625,27 @@ http_hdr_type_set(HTTPHdrImpl *hh, HTTPType type)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
-void
+bool
+is_version_supported(const uint8_t major, const uint8_t minor)
+{
+ if (major == 1) {
+ return minor == 0 || minor == 1;
+ }
+
+ if (major == 2) {
+ return minor == 0;
+ }
+
+ return false;
+}
+
+bool
+is_http_hdr_version_supported(const HTTPVersion &http_version)
+{
+ return is_version_supported(http_version.get_major(), http_version.get_minor());
+}
+
+bool
Review comment:
This function doesn't return a value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#issuecomment-847444472
> It seems like the parser still accepts an invalid request line like `GET / HTTP/2.0` since 2.0 is "supported". Shouldn't we have only `1.1` and `1.0` for the restriction?
Hmm, yeah, good point. I'd be okay to remove 2.0 if you prefer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#issuecomment-847445953
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#discussion_r638374471
##########
File path: proxy/hdrs/HTTP.cc
##########
@@ -625,7 +625,27 @@ http_hdr_type_set(HTTPHdrImpl *hh, HTTPType type)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
-void
+bool
+is_version_supported(const uint8_t major, const uint8_t minor)
Review comment:
Added a comment. I think we can update this when 3.0 is supported
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on pull request #7875: Restrict HTTP versions allowed on the HTTP request line
Posted by GitBox <gi...@apache.org>.
sudheerv commented on pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#issuecomment-847472511
> I don't have a strong opinion on whether we allow the invalid request line. However, I think the restriction should be either a) accept all the versions (i.e. `1.1`, `1.0`, `2.0` and `3.0`) that are on the RFC you quoted && can be handled by ATS , or b) accept only `1.1` and `1.0` that are the only valid versions in reality.
>
> Option a) could be complicated. While ATS has experimental support for H3, it is only available if ATS is built with a SSL library that supports QUIC (special OpenSSL or BoringSSL). We may want to have `ifdef` in the function if we take option a). This is why I wanted you to clarify what you mean by "supported". Strictly speaking, we should check whether ALPN (or NPN) is available to say `2.0` is supported in that sense.
>
> My biggest question was why the function doesn't allow 3.0 where it allows 2.0, but now I'm inclined to option b) because of the complexity above. I don't see benefits to have the complicated function at the moment.
+1
Agree with the reasoning. Updated to remove HTTP/2.0 as well for consistency.
Thanks for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv removed a comment on pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
sudheerv removed a comment on pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#issuecomment-847445953
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv merged pull request #7875: Restrict HTTP versions allowed on the HTTP request line
Posted by GitBox <gi...@apache.org>.
sudheerv merged pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#issuecomment-847442787
It seems like the parser still accepts an invalid request line like `GET / HTTP/2.0` since 2.0 is "supported". Shouldn't we have only `1.1` and `1.0` for the restriction?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] sudheerv commented on a change in pull request #7875: Enforce HTTP parsing restrictions on HTTP versions supported
Posted by GitBox <gi...@apache.org>.
sudheerv commented on a change in pull request #7875:
URL: https://github.com/apache/trafficserver/pull/7875#discussion_r638374378
##########
File path: proxy/hdrs/HTTP.cc
##########
@@ -625,7 +625,27 @@ http_hdr_type_set(HTTPHdrImpl *hh, HTTPType type)
/*-------------------------------------------------------------------------
-------------------------------------------------------------------------*/
-void
+bool
+is_version_supported(const uint8_t major, const uint8_t minor)
+{
+ if (major == 1) {
+ return minor == 0 || minor == 1;
+ }
+
+ if (major == 2) {
+ return minor == 0;
+ }
+
+ return false;
+}
+
+bool
+is_http_hdr_version_supported(const HTTPVersion &http_version)
+{
+ return is_version_supported(http_version.get_major(), http_version.get_minor());
+}
+
+bool
Review comment:
Ugh, copy paste error from internal repo to ASF. Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org