Only allow specific trailers#199
Conversation
Only allow trailers listed in the `Trailer` header. Even if listed there, disallow the following names, based on Mozilla recommendations: ``` content-encoding content-type content-range trailer authorization set-cookie transfer-encoding content-length host cache-control max-forwards te ``` There are probably additional ones we should disallow, but this is a decent start. Do not merge the header and trailer data. Parse the trailers into a separate hash, and for allowed names, copy the value into the headers hash. This ignores invalid trailers instead of raising an exception, which is preferable for backwards compatibility. In order to get the new test to pass, make content_length return nil instead of of raising a TypeError if no content length was provided. Also, parse the content length as decimal instead of trying to autodetect the radix. Fixes ruby#198
|
Hey @jeremyevans, could it be merged, please? And bundler-audit reports it as a vulnerability: Thanks! |
|
I was hoping for another committer to review, but then noticed I didn't request a review. Sorry about that. @ioquatix could you please review so I can merge this? The fact that 15 people gave a thumbs up on this comment in 45 minutes is discouraging. If you are using webrick in an environment where this pull request would matter, you should move to another Ruby webserver as soon as possible. I understand that such a migration can be challenging, but if you value security at all, you should not be using webrick in production. Webrick is not for production use, as stated in the README. Maybe we should change the wording to more strongly discourage production use. |
100%. Just commenting to reassure that the interest, for us at least, is more about unblocking CI pipelines. For the moment we're ignoring this particular CVE via ---
ignore:
- CVE-2026-38969Big thanks to the maintainers ❤️ |
|
@odlp Thank you, that makes me rest easier :) . I can certainly understand using webrick for testing in CI, as it is pure Ruby, and most other Ruby webservers rely on a native extension. |
Thanks @odlp! For anyone using the default github actions from Rails, use |
|
Thanks for working on this issue. I'm on the same boat as @odlp Looking forward to seeing this merged and deployed. |
|
I do not know who registered CVE-2026-38969, but this is an act that undermines the ecosystem. I will submit a request to MITRE to have it rejected. |
|
@hsbt How do you feel about reviewing and approving this PR and releasing a fix to help those blocked by this? |
I cannot understand how anyone is "blocked" by this. But people roaming around the internet asking for sudden action by maintainers "because some tool said" really bugs me. The code is the same as it was yesterday. If you're not using webrick (and it's just there as a dep) there is no vulnerability. Change your silly policies and suppress as "not vulnerable" or "no fix available". This isn't rocket science. CVEs aren't "truth". They are "possible vulnerabilities, to be risk adjusted by review within your environmental context".
Perhaps the original security researcher quart27219 via #198 - assuming it was not additionally responsibly disclosed via another channel. |
@jeremyevans The underlying problem is that Rails pulls in webrick via railties -> rackup -> webrick. The scanners don't care whether you actually use it or not, hence all the reports. Maybe rackup should remove the dependency on webrick? EDIT: Actually I just noticed latest versions of rackup (2.2+) do not depend on webrick, but that being said, for some reason on my machine, even on Rails 8.0, it still resolves rackup to 1.0.1, which does include webrick - looking into why. EDIT 2: Ok, so in our case, we are still on sprockets 3.x, which has a "rack < 3" constraint and railties has "rackup >= 1.0.0". This combination ends up forcing rackup 1.0.1, but prevents allowing rackup 2.0+, thus causing webrick to be installed. So, it's likely others are running into a similar constraint forcing an older version of rackup. |
That's largely due to the lack of an install-time peer/optional dependency constraint (or exclusion) system within Rubygems/bundler, as far as I can tell - a feature that is common in all other ecosystems I can think of (for better or worse as to implementation). I create empty local-only stub gems to solve problems like this in Ruby with end-user applications; which comes with its own problems/risk. Library authors are forced to construct hacky runtime checks, or have users install much more than is needed via spurious compile time dependencies. railties creates many issues due to things like this. But that's what suppressions are for, and not much that a downstream dependency can do about that.
Depends on your Rack version, usually. You need to get to rack 3 before you can use rackup v2, so if you are still on rack 2.2 for some reason, you'll be stuck with rackup v1, and the (spurious?) webrick dependency. |
For me, the issue isn't rackup, but prometheus_exporter. Most of my apps use it for metrics. |
Sadly, I believe that one is another victim of the lack of optional/peer dependency constraints in Ruby. (just from memory, cannot recall if the maintainers specifically rejected the runtime failure mode that other gems use - but there are many good reasons to do so) |
@chadlwilson The PR has been waiting for review since March. No-one is asking for sudden action. It is unclear whether the maintainers of It would be helpful to communicate which is the chosen path forward. |
I'm one of the maintainers and I created this PR in response to #198. In case it wasn't obvious, we are planning to fix the issue. However, the PR should be reviewed before it is merged. The main reason this seems to be a problem:
Unfortunately, automated security scanners lack nuance. I assume most users of webrick using webrick only for testing/CI just mark the security scanner reports for webrick as not affecting their applications, since that seems to be the proper course of action. Nobody so far has reported that they are using webrick in production, which is a relief. If you are using webrick in production, you should immediately transition to another Ruby webserver. If you are using webrick for testing/CI, just mark the security scanner reports as not affecting your application. |
This is quibbling about language semantics you may not have intended, but using terminology like "...blocked by this" is loaded language which implicitly asserts responsibility on the group/person whose action is implied needed to "unblock", along with urgency (blocking implies "my work cannot continue"). I'm disputing both the framing (no-one is actually blocked, unless it's by their own policies rather than this issue) and the implication (smuggling in a sense of urgency). If you're not looking for sudden action, though, that's good. I understand people use the term loosely with respect to pipelines/builds which "block" build/deployment on new vulnerability discovery (a poor, but sadly common industry pattern), but if that's the reason for using the term 'blocked', you (or your org) has shot your own foot there rather than this project and you have agency to unblock yourself, hence my assertion that I can't understand why anyone is blocked by "this" issue. |
|
First and foremost, I would like to sincerely thank the maintainers. I apologize if my initial request came across as a demand. My intention was merely to point out that there is an issue, nothing more. Regarding the usage of the webrick. It might be a little broader than test and development environments. For example, the yabeda-prometheus gem is using So in most cases those services are not exposed to the open internet, which might be a huge security problem whenever The reason why some devs (including myself) choose a |
This may not seem like a huge security problem, but then you have a SSRF issue in your application, and suddenly what you thought was not publicly accessible becomes publicly accessible. If you are using webrick in this capacity, you should transition to another Ruby webserver. If libraries are using webrick in this capacity, they should transition to another Ruby webserver. There are other Ruby webservers that can be used, such as puma and falcon, either of which would be much better choices than webrick. Development and testing are about the only places you should even consider using webrick, and most of the time you would be better off using puma or falcon in those cases as well. |
|
I filed rubysec/ruby-advisory-db#1164 and requested rejection of CVE-2026-38969 to MITRE. |
| unless requested_trailers.empty? | ||
| trailers = HTTPUtils::parse_header(trailer_lines.join) | ||
| requested_trailers.each do |key| | ||
| if value = trailers[key] |
There was a problem hiding this comment.
parse_header returns Hash.new([].freeze), so trailers[key] never returns nil. When a name is listed in Trailer but not actually sent as a trailer, this overwrites an existing header of the same name with an empty array. I think this should check trailers.key?(key) instead.
| raise HTTPStatus::BadRequest, "invalid content-length request header" | ||
| if trailer_lines | ||
| if requested_trailers = self["trailer"] | ||
| requested_trailers = requested_trailers.downcase.split |
There was a problem hiding this comment.
Trailer is a comma-separated list per RFC 7230, and self["trailer"] joins multiple lines with ", ". Splitting on whitespace turns Trailer: Expires, X-Foo into ["expires,", "x-foo"], so expires never matches. split(/[,\s]+/) would handle both forms.
Just curious - what's the basis for having them rejected if the team agree it is a vulnerability in some context? Just out of principle, since MITRE created it with no validation? Are you intending to raise your own CVE rather than try and "get control" of this one? Subsequent to this, the US CISA has scored it as well, so they seem to think it's real. No idea what due diligence they perform. https://www.cve.org/CVERecord?id=CVE-2026-38969 I've been told by MITRE before that without becoming CNA for ones own scope of vulns one has no rights other than to have something marked as "disputed". Perhaps Ruby is CNA for everything under the umbrella though, which provides more leverage for you. I'm likely as annoyed as you at MITRE and the ludicrous process to get stuff there for clout, based on no technical validation - however since CVSS doesn't make any assertions about environment - and indeed that is a separate scoring dimension - the position would normally be that a vulnerability is just that - a vulnerability. Isn't that valuable to know, even if the process was bad to get here? Even if webrick is not meant for production use now it certainly used to be, right? So highlighting a vuln still seems valuable - even if you disagree on severity. There are people in this thread using it in production, even if they shouldn't be. Of course if you disagree it's a vuln (or more than a bug) then it'd be useful to see that articulation somewhere. Apologies if I missed it, but I don't find this particularly compelling:
Is this not in fact an opening to request smuggling as described in the issue, from your perspective? For the record I agree this creates a lot of noise, and the process was not good here - so that's not good, but in my view that is due to the limitations of Rubygems/bundler itself not having a canonical way to express test or optional dependencies such that tools/users can score them appropriately (or ignore vulns from test deps), as opposed to what is possible in other ecosystems. That is how other systems bring in a proxy for the "environmental" risk modifier that CVSS has. Thus the noise is partly due to Ruby ecosystem itself, rather than a solid justification for rejecting the vuln wholesale (assuming it is a vuln in some realistic contexts). |
|
@hsbt Thank you very much for your review. I've pushed a commit that addresses both of the issues you found. |
Trailer is a comma separated list, not space separated, so consider that when splitting. parse_header returns a hash that uses an empty array as a default value, so use key? to check whether the trailer contains the entry.
jeremyevans
force-pushed
the
chunked-trailers
branch
from
f4f22a8 to
6e60c7d
Compare
10 participants
Only allow trailers listed in the
Trailerheader. Even if listed there, disallow the following names, based on Mozilla recommendations:There are probably additional ones we should disallow, but this is a decent start.
Do not merge the header and trailer data. Parse the trailers into a separate hash, and for allowed names, copy the value into the headers hash.
This ignores invalid trailers instead of raising an exception, which is preferable for backwards compatibility.
In order to get the new test to pass, make content_length return nil instead of of raising a TypeError if no content length was provided. Also, parse the content length as decimal instead of trying to autodetect the radix.
Fixes #198