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

openssl 'BC' (security provider) leak #1543

Closed
wants to merge 84 commits into from

Conversation

Projects
None yet
3 participants
@kares
Copy link
Member

kares commented Mar 4, 2014

there's an issue around

java.security.Security.addProvider(BC_PROVIDER);
... it might be causing (unpredictable) leaks esp. under environments where multiple Ruby runtimes load their own BC classes and execute addProvider.

the only solution is to review all security "getInstance" factory calls and avoid everything that depends on 'BC' being registered (e.g. NetscapeCertRequest), which is what happened here with some additional cleanup since I was unable to read some parts of the code (in a shallow attempt to understand what's it doing) ... let me know what do you guys think. I also have this ready for jruby-1_7 since that is what I'm actually be putting onto production :)

I kept the commits (well I did some squashing-re-arranging of mine) as they are so that anyone following the log better understands why things are the way they are.

kares added some commits Feb 28, 2014

implement BC specific (factory) helpers using BC APIs "directly"
we're trying to avoid registering the java.security provider since it's problematic in environments with multiple runtimes started

+ start implementing our own [security].getInstance factory methods
setup our own getMac factory and use it + some minor HMAC cleanup
- mostly do not catch and hide argument to_s conversion errors raised
deprecate doWithBCProvider and avoid it in code base (NetscapeSPKI)
generixize getWithBCProvider's Callable lambda interface
setup tests for our factories and tune them out + initialize BC when …
…ext loads

unfinalize constants (for tests) and only call BC-factories if provider != null
Cipher.getInstance gets tricky due JCE internals - but seems fine the…
… API way

it depends on which provider's classes are loaded with the SunJCE's one it won't work since the internal `JceSecurityManager.INSTANCE.isCallerTrusted()` fails
- but Sun's getCipher(transformation, provider) works without any assumptions being made for the provider to be registered among the Security providers ...
move factories into a new SecurityHelper - allows to change/register …
…provider

- for slightly extreme cases where users want to force using a given provider (and they of course understand the drawbacks)
- also if registering a provider is desired there's a public method to be called
@kares

This comment has been minimized.

Copy link
Member Author

kares commented Mar 4, 2014

OK, this still isn't enough - the 1.47 PEMReader does https://travis-ci.org/jruby/jruby/jobs/20046780#L881

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Mar 12, 2014

from https://travis-ci.org/jruby/jruby/builds/20241315 you see if you run
$mvn -Pmain -Dinvoker.skip=false
that there are side effects if you run jruby with an old jruby-openssl gem installed. I will have a look at that if I can find something.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Mar 12, 2014

@mkristian yep I noticed ... just hadn't had time to review those (probably a "refactoring" regression)
... it's definitely on my list, but I appreciate any reviews or feedback I get as it is already. so thanks!

kares added some commits Apr 23, 2014

add a spec for `OpenSSL::PKey::RSA.new` regression - working fine as …
…is (fixes #357)

not tested on Mac (only Linux with Java 1.7.0_51) but we're now not simply doing `Cipher.getInstance(String)` ... we're first attempt to consult (BC) provider directly
de-ja-vu BC 1.49 (binary) compatibility while still maintaining 1.47 …
…compatibility

- a better 941ab04 (part) which got reverted
- tested #1238 regression - fine on 1.47 as well as 1.49 with this change
@kares

This comment has been minimized.

Copy link
Member Author

kares commented Apr 23, 2014

... and at last JRuby-OpenSSL is now BC 1.47-1.49 compatible. in can easily be 1.50 compatible as well (and probably is just it does not compile against BC 1.50) it requires removing a deprecated class, but just in case I would first do a 0.10.0 release before going further. as noted before it's JRuby 1.6.x / 1.7.x compatible and I've changed the pom.xml to build using javac --target 1.6 ... the static map leak in ASN1 has also been handled (using a WeakHashMap) ... let me know if there's anything more to be done here.

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Apr 23, 2014

merged to test-openssl-bc-leak and once that is green (enough) I will merge to master.

please also have a look at bd519e7 since that was a conflict which I am not sure in which way to resolve it. with this commit it follows master.

@mkristian mkristian closed this Apr 23, 2014

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Apr 23, 2014

thanks! ... I see that BN revert is actually something we do not want (not 1.6.8 nor 1.7.x compatible) ...
just take the latest of what's in on my side kares@0e69ad0 (it's been reviewed and I just did look again)

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Apr 23, 2014

On Wed, Apr 23, 2014 at 8:30 PM, Karol Bucek notifications@github.comwrote:

not 1.6.8 nor 1.7.x compatible

the extension gets now compiled against jruby-1.6.8, just to understand
what you mean with not compatible ?

it was not my code but someone did those changes and then I changed them to
work with jruby-1.7 a while ago. so I would like to keep. but if I
understand your issue I will revert it.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Apr 23, 2014

ClassIndex is not available all the way down to JRuby 1.6 ... had issues with it with non-recent 1.7 as well.
it's pretty straightforward to re-implement without the ClassIndex ... not worth the hustle of keeping it (you should have seen a failure already esp. if you're compiling against 1.6.8's API) ... the rebased versions gets a lot of failures but here's how it went for me with an older base commit https://travis-ci.org/kares/jruby/builds/23575841

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Apr 24, 2014

thanx for explaining it more. I still do not see the concrete problems but
I can not test it against against 1.6.x (it compiles but not sure if it is
working as expected). I will roll it back and use the if-else code then ;)

On Wed, Apr 23, 2014 at 9:49 PM, Karol Bucek notifications@github.comwrote:

ClassIndex is not available all the way down to JRuby 1.6 ... had issues
with it with non-recent 1.7 as well.
it's pretty straightforward to re-implement without the ClassIndex ... not
worth the hustle of keeping it (you should have seen a failure already esp.
if you're compiling against 1.6.8's API) ... the rebased versions gets a
lot of failures but here's how it went for me with an older base commit
https://travis-ci.org/kares/jruby/builds/23575841


Reply to this email directly or view it on GitHubhttps://github.com//pull/1543#issuecomment-41212878
.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Apr 24, 2014

let me know how I can help - all should be fine, however with the new base I can not test the old way either - due to mvn now complaining (have not been looking into that as I started writing some repo local tests to fix the issue and it's much simpler to test that way ... should have not modified openssl imported tests anyways - no worries there I'll submit another PR for this as this one gets handled).

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Apr 24, 2014

do you mean the mvn test does not run the junit tests ? hmm - that is my fault. I will fix this. I also reverted that the commit of mine back to how it was from the PR

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Apr 24, 2014

@mkristian no that's not that important I meant mvn -P main -rf ... from #1543 (comment) seemed no longer usable (but as I mentioned I do not need it and travis clearly has some other issues as is)

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Apr 30, 2014

@kares There are a few things that should be addressed in this fix and most of them are actually the same thing. I am not sure how many of these public Java methods are used beyond their @JRubyMethod bindings but removing ThreadContext broke backwards compatibility (You also renamed some initialize methods like _initialize -> bm_initialize in BM.java).

The other reason for keeping ThreadContext on these methods is that context passed through is much faster at retrieving runtime than calling getRuntime(). In fact we started phasing out uses of IRubyObject.getRuntime() from our codebase (although there are still a ton of them).

I know this is a big item but as a followup could you restore all signatures (which seem to mostly revolve around initialize) to their previous values (as a followup PR). Sorry, I wish I had gotten into this review process to save you the pain...

@mkristian This note will be useful for you to read in merging 1.7 branch stuff (and usefulness of ThreadContext).

@kares

This comment has been minimized.

Copy link
Member Author

kares commented May 1, 2014

@enebo thanks, actually I tried doing the thing you mention - pass in context (it was almost never passed) esp. with methods that use it extensively and previously did a getRuntime().getCurrentContext() ... compatibility seemed fine - tried tests against 1.6.8 (did compile all the way down to 1.6.4 - did not try further).

who else could be using the JRuby-OpenSSL @JRubyMethod Java API (if there's anything concrete you had in mind) ?

those BN changes were a bit of an accident but by the time I noticed I left it as is ... I'll be happy to restore those if it's that important. for others such as _initialize I mostly added a ThreadContext param, so those will need to get back as well (or all of them need to get back to the state where context is not passed - if so let pls do let us know why or what compatibility we need to test with these) ?

thanks again for looking into this - much appreciated, I'll be off a week or so but would be happy to get it ironed out once I'm back.

also @mkristian told me this will not need to get "back-merged" to 1_7 (or anywhere else - I actually did the merge there were no "huge" conflicts) - as releases will be done from master and the pom actually compiles against JRuby 1.6.8 + Java 6 ... maybe you guys need to talk figuring this out :)

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented May 1, 2014

@kares, @enebo hey just some info regarding the build system of jruby. the
build in both master and jruby-1_7 branch will pick a "released" gem to
include it as default gem into the stdlib (see lib/pom.rb). i.e.
technically the /ext/* directories could live in its own repository. the
link to the jruby build is the gem from rubygems.org or
oss.sonatype.org(for snapshots). for releasing a gem we need only one
branch !!

regarding backward compatibility I picked that is much more then just
getting it compiled against jruby-1.6.8. so once we have a snapshot release
of that gem we can use it for the jruby-1_7 branch or just make a
test-jruby-openssl-with-jruby-1_7 with that gem. might even make sense for
master . . . . not sure right now

@enebo

This comment has been minimized.

Copy link
Member

enebo commented May 1, 2014

@kares I actually read the merge backwards. ignore most of my backwards compat issues. I thought this was landed on jruby-1_7 and not on master. :) The merge was just so large and I changes to ThreadContext from public signatures. Perhaps I just reversed everything in my head?

So don't bother to change anything now. Once I have completed a merge between jruby-1_7 and master I will change anything which I notice. As @mkristian said, we only release one gem from either branch (and it probably should be moved to its own repo). So long as this will load from jruby-1_7 we should probably continue to use master repo.

@enebo

This comment has been minimized.

Copy link
Member

enebo commented May 1, 2014

There is a potential wrinkle that some APIs for native extensions might actually change in JRuby 9000...Hmmm, perhaps any new APIs we can backport to 1.7.x and then major rev jossl and it will force people to update to later 1.7 version to get ossl fixes past a particular point?

@enebo

This comment has been minimized.

Copy link
Member

enebo commented May 1, 2014

@kares To answer your question I am unsure if anyone will directly hit the Java APIs in jossl which was my original fear that if someone does then a jossl release will break them. I guess if it seems extremely unlikely the handful of initialize methods can change name. If not we should change them back.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented May 7, 2014

OK, thanks for the replies ... if anything let me know what needs more (reverting) work or if you guys need help. I tried to clean upcode as I was into it trying to understand what goes on ... and since a quite some parts depended on security APIs directly alot got changed (hopefully for the better :). There's also another PR on top of this that fixes an issue and improves ext.value performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.