Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align behavior with CRuby for security specs #6304

Closed
headius opened this issue Jun 29, 2020 · 8 comments
Closed

Align behavior with CRuby for security specs #6304

headius opened this issue Jun 29, 2020 · 8 comments

Comments

@headius
Copy link
Member

headius commented Jun 29, 2020

In #6303 @eregon noticed a few of the security specs do not pass currently.

Tags are here: 48c5e85

An example run is here: https://travis-ci.org/github/jruby/jruby/jobs/702869188

The failures are mostly due to us not randomizing hash calculation, but there's a few others that warrant a second look. I believe it would be best for us to patch these for the 9.2.12 release this week.

@headius
Copy link
Member Author

headius commented Jun 29, 2020

The hash failures are all due to us not using siphash plus a random seed in the numeric classes. As with other places this was patched, we have not generally agreed with the "fix" of randomizing hashes, and instead allow users to opt into siphash if they have concerns. That decision could be revisited, but at least we should update the numeric classes to also have optional siphash calculation. We may also be able to use our existing "perlhash" with a random seed and meet the spirit of these fixes.

The WEBrick failures appear to have never been made to the ruby/webrick repository, or at least Ruby 2.6 claims to ship WEBrick 1.4.2, but there are extensive diffs from the ruby_2_6 branch and the WEBrick v1.4.2 tag. It appears CRuby has diverged here without properly updating release versions. See ruby/webrick#48.

The Array#pack failure shows a place where we are not propagating tainting. We have never fully supported tainting, as it is a broken security mechanism. Nevertheless, we should be able to patch this easily to pass the spec. I assume this spec will no longer be relevant when tainting is removed as a feature.

The String#unpack failure simply raises the wrong error. There's no vulnerability here (both because of the error and because the JVM will prevent such memory issues), but we could add the logic to raise the RangeError the spec expects.

headius added a commit to headius/jruby that referenced this issue Jun 29, 2020
This corrects the error raised for the case where the unpack spec
requests a size that's outside the string, which relates to
CVE-2018-8778.

The other two cases that raised the ArgumentError for "outside of
string" appear to be correct, since making them raise RangeError
causes spec failures.

This addresses a security spec failure as shown in jruby#6304.
headius added a commit to headius/jruby that referenced this issue Jun 29, 2020
CRuby modified the hashing of core numeric types to incorporate a
random seed, as part of the flurry of "hashDOS" fixes in the early
2010s. This commit mimics those changes.

See jruby#6304 for the failing security spec.
headius added a commit to headius/jruby that referenced this issue Jun 29, 2020
CRuby modified the hashing of core numeric types to incorporate a
random seed, as part of the flurry of "hashDOS" fixes in the early
2010s. This commit mimics those changes.

See jruby#6304 for the failing security spec.
@headius
Copy link
Member Author

headius commented Jun 29, 2020

The numeric hash failures are addressed in #6305.

The String#unpack error is addressed in #6306.

@headius
Copy link
Member Author

headius commented Jun 29, 2020

The Array#pack failure is due to us not tainting the result when any of the elements of the array are themselves tainted.

The modifications to fix this would be somewhat invasive.

Given the fact that:

  • tainting is deprecated in 2.7 and to be removed in 2.8/3.0
  • tainting has never been fully supported in JRuby
  • tainting is in fact a broken security mechanism

...I vote that we don't bother with this change.

@headius
Copy link
Member Author

headius commented Jun 29, 2020

The remaining WEBrick failures will be addressed once we figure out what version of WEBrick is actually being shipped in Ruby 2.5.x and 2.6.x. CRuby appears to be shipping heavily patched sources for both releases, while claiming they're both WEBrick 1.4.2.

ruby/webrick#48

@eregon
Copy link
Member

eregon commented Jun 29, 2020

The 2 PRs seem green now.
Should they also be applied to master, along with removing no longer needed spec tags?

@headius
Copy link
Member Author

headius commented Jun 29, 2020

@eregon We periodically merge jruby-9.2 to master. The spec tags can be removed at that point.

@headius
Copy link
Member Author

headius commented Jun 30, 2020

All security spec failures have been addressed:

@headius headius closed this as completed Jun 30, 2020
headius added a commit that referenced this issue Jun 30, 2020
@headius
Copy link
Member Author

headius commented Jun 30, 2020

Spec tags have been updated on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants