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

BDSA-2018-5235 raised for bouncycastle 1.6.8 #925

Closed
erezul opened this issue Apr 6, 2021 · 29 comments
Closed

BDSA-2018-5235 raised for bouncycastle 1.6.8 #925

erezul opened this issue Apr 6, 2021 · 29 comments

Comments

@erezul
Copy link

erezul commented Apr 6, 2021

https://fis.blackducksoftware.com/api/vulnerabilities/BDSA-2018-5235/overview
Bouncy Castle contains a weak key-hash message authentication code (HMAC) that is only 16 bits long which can result in hash collisions. This is due to an error within the BKS version 1 keystore (BKS-V1) files and could lead to an attacker being able to affect the integrity of these files.

Note: This issue issue occurs due to functionality that was re-introduced following the fix for CVE-2018-5382 (BDSA-2018-1190).

@RPCMoritz
Copy link

I believe that BlackDuck is falsely flagging this as a "high" severity issue - but it may also be time to remove the legacy implementation.

Be aware that the link you posted is not reachable for anyone outside your organization.

If you could raise an issue with your BlacDuck-Provider to re-evaluate the issue (some warning, that you can use an existing implementation to build insecure software would be helpful - but it's certainly not a high security risk, to use bouncycastle in general), I would be very grateful.

For the maintainers: I'm guessing this is on the roadmap for r2?

@RPCMoritz
Copy link

I also haven't been able to find a rationale for the re-enabling back in 2013 (I'm assuming it was plenty of people running into issues on managed platforms?). If there is one to be recovered - does it still hold today?

@erezul
Copy link
Author

erezul commented Apr 7, 2021

Thanks for your comments.
Yes, it seems to be only in BlackDuck for now - no CVE yet.
They say there is a fix in 1.68.0.redhat-00001 for bcprov-jdk15on:1.68 but not for bcpg-jdk15on:1.68
Yes, the link seems to be internal. pasting a screenshot:
image
Yes, I can approach blackduck with a response. do note their interest is to find many high vulnerabilities...
Could you provide more detailed reasoning?

@meganwoods
Copy link
Contributor

Hi,

Are you using BKS-V1?

MW

@erezul
Copy link
Author

erezul commented Apr 7, 2021

No. but for blackduck it does not matter.
If for instance there is old code within the same jars the only solution would be to split the jars to old and mew.

@dghgit
Copy link
Contributor

dghgit commented Apr 7, 2021

So, the backwards compatible type was introduced as people needed to generate keystores that were accessible by legacy applications and devices, particularly devices. I'm not sure how many people would still need it now, but our experience with similar things suggests the number will not be zero.

The BKS-V1 format is documented as only offering a weak checksum, so there won't be a CVE, anymore than we would introduce a CVE because we read PEM files, Armored PGP records, or ASN.1 data. The actual CVE (CVE-2018-5382) that gave rise to this is about what happened when you asked for a BKS keystore before BC 1.47. You got a store with a reduced checksum of 16 bits when it was supposed to be 160 bits (while BKS was only ever about offering JKS level of security, this is clearly not what we had in mind).

So the situation we are now facing is that both Veracode and BlackDuck are now red flagging the provider because it supports something that is documented as doing what it actually does. I'll be honest - I am not really sure that makes any sense to me.

That said, I am now spending a considerable amount of my time having to deal with this. Apart from multiple github issues, I am getting multiple emails as well. For 1.69 we have added a property that must be set to true to enable BKS-V1, this can also be done in java.security to make sure it is not enabled. Hopefully that will calm things down. If it doesn't we will have to remove the functionality in 1.70 as this is causing far too much stress to our users and is wasting far too much of our already non-existent time. I've no doubt what the tool providers are doing is well intentioned, however, as the old adage goes, sometimes that lays the path to hell for others...

@erezul
Copy link
Author

erezul commented Apr 7, 2021

I feel the same.
Since my company picked blackduck I MUST upgrade libraries every month...
For them it does not help that you have a flag to enable something, or that it is documented as weak. If it exists, hacker might exploit it. There were cases were I had to remove parts of a jar to avoid the vulnerability...
My other option is to request a mitigation by explaining I'm not using this functionality... However, then I need to request it again for each release, even if using the same library version.
Having said that, I would prefer getting a jar with no old stuff. the current one is jdk5on. maybe its time for jdk8on?

@RPCMoritz
Copy link

So the situation we are now facing is that both Veracode and BlackDuck are now red flagging the provider because it supports something that is documented as doing what it actually does. I'll be honest - I am not really sure that makes any sense to me.

I requested a review of the vulnerability via my BlackDuck contact - but of course, it's unlikely to happen. This would be like flagging any instance of curl as dangerous, because there's an option -k.
I see the point of warning users of this, but anything other than a low or medium severity does appear to be exaggerated.

But I do agree that a removal at this point in time is likely going to be a net win, compared to back when the CVE was first raised, as there was more time for legacy apps to be fixed - and if not, then anyone depending on the vulnerable code will be rightly flagged by BlackDuck.

@RPCMoritz
Copy link

And the link to the Veracode issue, just for posterity; #850

@dghgit
Copy link
Contributor

dghgit commented Apr 7, 2021

I'm hoping we'll get a clearer picture of the situation with 1.69 - I'd expect we'll hear from some people who might be affected by the change if they're using BKS-V1.

The main issue is actually Android devices - in 2018 84% of the BKS files seen in Android environments were still in the old BKS-V1 format. At the time the provider of the research, Will Dormann from CMU, was unable to determine why. CVE-2018-5382 was introduced as a result of that research as by then we were actually able to raise CVE as we had a the charity registered. The CVE was raised on the assumption that better publicity would further reduce any accidental use of the files. At any rate, that's still only a couple of years ago which may mean people still need to be able to produce the files.

@erezul
Copy link
Author

erezul commented Apr 7, 2021

any idea when 1.69 will be released?

@dghgit
Copy link
Contributor

dghgit commented Apr 7, 2021

At the moment we're looking at June.

@dghgit
Copy link
Contributor

dghgit commented Apr 7, 2021

@RPCMoritz - one thing, I have to admit I am also quite interested in why it gets such a high rating. It's almost as if they think private data could be compromised, which is not the case, the original BKS issue only affected the keystore checksum, not private key encryption. There has been confusion about this in the past as well.

@dghgit
Copy link
Contributor

dghgit commented Apr 8, 2021

One other thing, in the meanwhile, if people need it I am happy to help explain to highers up why these flagging are in error. If I'm going to waste my time on this, I might as well try and achieve some good by doing so. Please get in touch with me at dgh at bouncycastle.org if assistance is required.

@erezul
Copy link
Author

erezul commented Apr 8, 2021

I can open a ticket to blackduck if you provide the reasoning...

@dghgit
Copy link
Contributor

dghgit commented Apr 8, 2021

I think it would not hurt if they provided the reasoning. They did not even notify us. That said, the file has a 16 bit checksum, it doesn't claim it has anymore than a 16 bit checksum. No private data in the file is compromised by the fact it has a 16 bit checksum. BKS-V1 essentially works as advertised. How is that a problem?

@erezul
Copy link
Author

erezul commented Apr 8, 2021

That is all they have to say, as they hold all the power:
Bouncy Castle contains a weak key-hash message authentication code (HMAC) that is only 16 bits long which can result in hash collisions. This is due to an error within the BKS version 1 keystore (BKS-V1) files and could lead to an attacker being able to affect the integrity of these files.

Note: This issue issue occurs due to functionality that was re-introduced following the fix for CVE-2018-5382 (BDSA-2018-1190).****

@dghgit
Copy link
Contributor

dghgit commented Apr 8, 2021

So, I can affect the integrity of a regular PEM file using a text editor. I'm not sure I see the difference here.

@dghgit
Copy link
Contributor

dghgit commented Apr 9, 2021

Okay, for anyone who is interested 169b05 which has the security property present is now available at https://www.bouncycastle.org/betas If anyone can get Black Duck (at least) to evaluate it and determine that it meets whatever it is that they are trying to do I would be grateful.

@erezul
Copy link
Author

erezul commented Apr 10, 2021

I don't believe they would evaluate a beta jar.
I also don't believe adding the security property would do any good.
If the vulnerable code is there its vulnerable. that is how they think...

@dghgit
Copy link
Contributor

dghgit commented Apr 10, 2021

This isn't about vulnerable code though. It's about a file format.

Okay, so I'll admit it looks grim. Both Veracode and BlackDuck have been happy to raise an issue in their systems that is not based on a CVE (CVE-2018-5382 is only listed for 1.46 and earlier, it was raised well after 1.49, and in the knowledge that the backwards compatibility code had been added), both Veracode and BlackDuck did not advise us that they were going to do this, nor did Veracode and BlackDuck give us any opportunity to either respond or adjust the way the BKS-V1 code was working to relieve the need to raise the issue. I'll let that speak for itself, but it would be fair to say they probably don't care.

At the moment our feeling is, if we can't get some co-operation on this, we think we will have to remove the functionality. We'll continue to support people that need it some other way. To everyone else, all I can say is I'm sorry, but the behavior of Veracode and BlackDuck on this one has completely blindsided us, we will get something sorted out for 1.69.

@dghgit
Copy link
Contributor

dghgit commented Apr 13, 2021

News! I've heard from Veracode, they agree that adding the property and having the default set to false is enough to satisfy them the use of BKS-V1 is adequately guarded. I feel suddenly optimistic, it appears they do care. If someone who has a contact with BlackDuck would get some feedback from them, we may be able to progress this.

@erezul
Copy link
Author

erezul commented Apr 13, 2021

I have submitted the following case to BD:
https://community.synopsys.com/s/case/5002H00001IfpeAQAR/bdsa20185235-raised-for-bouncycastle-168
your email, dgh@cryptoworkshop.com, is marked in cc

@prabodhshrivastava
Copy link

prabodhshrivastava commented Apr 15, 2021

@dghgit could you please plan to patch this. I know you mentioned this will be addressed in 1.6.9 sometime in June. But we need it before May.
Patch=1.6.8+this fix ~ 1.6.8.1

As you already found the fix it'll be really appreciated
Please let me know how can I help you

@dghgit
Copy link
Contributor

dghgit commented Apr 17, 2021

@prabodhshrivastava we still haven't heard back from BlackDuck. I should point out what we've described isn't a fix to the provider though as well. The warning is about the use of a particular file format, it's not about the actual provider code - we're simply making it harder for people to use the files that are causing concern.

@dghgit
Copy link
Contributor

dghgit commented Apr 21, 2021

We've just heard back from BlackDuck they also agree that the presence of the System property removes the need to raise the issue. I am very relieved.

@RPCMoritz
Copy link

Thanks, their re-evaluation has already been synched with our instance.

@dghgit
Copy link
Contributor

dghgit commented Apr 26, 2021

We will try and get it out soon, there's a few things in process at the moment though.

@dghgit
Copy link
Contributor

dghgit commented Jun 6, 2021

BC 1.69 is now available in the latest releases area. It should start appearing on Maven in the next couple of days as well.

@dghgit dghgit closed this as completed Jun 6, 2021
toweroy added a commit to toweroy/helios that referenced this issue Jul 26, 2022
Bump `org.bouncycastle` depdendencies from `1.59` to `1.66` for security vulnerability fix regarding hash collision. More info:
 - https://insights.sei.cmu.edu/blog/the-curious-case-of-the-bouncy-castle-bks-passwords/
 - bcgit/bc-java#850
 - bcgit/bc-java#925
 - Changes between `1.59` and `1.66`: bcgit/bc-java@r1rv59...r1rv66
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

5 participants