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

Tell OpenSSL that MD5 is not used for security #232

Closed
wants to merge 1 commit into from

Conversation

rbclark
Copy link
Contributor

@rbclark rbclark commented Jan 11, 2019

This allows bro to run properly on a properly on a FIPS-enabled system. It should not change any actual functionality of Bro.

@0xxon
Copy link
Member

0xxon commented Jan 11, 2019

Hi,

thanks a lot for this (again).

I have not looked at this deeply yet - but one of the first things that I noticed is that the internal_md5 function looks like a copy of the OpenSSL MD5 function from MD5_one.c

Is copying it really necessary? From what I remember about OpenSSL and MD5, we should be able to call the MD5 function when there is a non-fips OpenSSL and the internal_md5 function when there is a fips-OpenSSL; would it perhaps be possible to change that function-copy into something that distinguishes between these 2 cases (based on defines) and then calls the correct function?

Otherwise we would probably have to at least add the OpenSSL copoyright headers to this file - and I kind of would like to avoid that if it is not absolutely necessary.

@rbclark
Copy link
Contributor Author

rbclark commented Jan 14, 2019

Thank you for taking a look at this! I would've definitely preferred to not have to copy the whole MD5 function from MD5_one.c however based on the limited information I could find about how EVP_MD_CTX_FLAG_NON_FIPS_ALLOW it seems that it needs to be set on a per MD5-context basis. Because of this I cannot reuse the existing MD5 function as far as I can tell, since I cannot set the flag for the context before calling MD5() and I can't modify the context after the call to MD5() since the call will have already failed. As for taking different codepaths for FIPS and non-FIPS calls, it is possible but functionally the code works as-is on both FIPS and non-FIPS machine.

@0xxon 0xxon self-assigned this Jan 22, 2019
@0xxon
Copy link
Member

0xxon commented Jan 22, 2019

FYI - I started refactoring this a bit last week and did not quite get done. This should be merged by thursday at the very latest.

@0xxon
Copy link
Member

0xxon commented Jan 24, 2019

Hi. I pulled this PR into a branch and made a bunch of other changes to it. Since someone else from the team should probably review that I opened another pull request at #255. I am going to close this - your commits are going to be merged in together with #255.

It would be neat if you could also take a look at the changes to your work I made in #255.

@0xxon 0xxon closed this Jan 24, 2019
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

Successfully merging this pull request may close these issues.

2 participants