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

Add a FIPS provider and implement SHA256 in it #8537

Closed
wants to merge 12 commits into from

Conversation

mattcaswell
Copy link
Member

This PR is built on top of #8513. It creates the FIPS provider and implements SHA256 within it.

So far this only includes the C implementation of SHA256. Adding the assembler modules is a bit trickier so that is deferred for a later time. A consequence of that is that (AFAICT) OPENSSL_cleanse is only implemented in assembler so (for now) I have implemented a very simple C version of OPENSSL_cleanse. It's totally unsafe to use in production so will need to be removed before too long, but it gets things working for now.

I encountered some unexpected behaviour during the implementation of this. At one point I had created the FIPS provider but not yet added all the files I needed for SHA256. Therefore I was expecting the test I had written to use the new provider to fail due to some symbols not being present. To my surprise the test passed - the module loaded and performed SHA256 sucessfully. It seems that, when building the FIPS provider, if there are symbols from libcrypto that it needs but that aren't present, those symbols are resolved automatically if the provider is loaded by an application linked against libcrypto. This is not desirable behaviour since the FIPS module must be entirely self-contained. To detect this problem I created a new test in shlibloadtest that simply loads the FIPS module from an application that isn't linked with libcrypto. If there are no missing symbols then this should work and will fail otherwise.

Finally, this PR also implements a "no-fips" Configure option.

@mattcaswell mattcaswell added the branch: master Merge to master branch label Mar 20, 2019
@mattcaswell mattcaswell added this to In progress in 3.0 New Core + FIPS via automation Mar 20, 2019
@levitte
Copy link
Member

levitte commented Mar 20, 2019

A consequence of that is that (AFAICT) OPENSSL_cleanse is only implemented in assembler so (for now) I have implemented a very simple C version of OPENSSL_cleanse.

[ahem] crypto/mem_clr.c

@levitte
Copy link
Member

levitte commented Mar 20, 2019

It seems that, when building the FIPS provider, if there are symbols from libcrypto that it needs but that aren't present, those symbols are resolved automatically if the provider is loaded by an application linked against libcrypto.

I wonder if this is true on all platforms... I think that Windows demands that all symbols be resolved when linking the DLL, and I know for a fact that this is the case on VMS.

@mattcaswell
Copy link
Member Author

Darn. I looked everywhere for that and couldn't find it!! Thanks for the pointer.

@levitte
Copy link
Member

levitte commented Mar 20, 2019

git grep is your friend 😉

@mattcaswell
Copy link
Member Author

I wonder if this is true on all platforms... I think that Windows demands that all symbols be resolved when linking the DLL, and I know for a fact that this is the case on VMS.

I could imagine that being highly platform specific

@mattcaswell
Copy link
Member Author

git grep is your friend wink

Too many hits to find the right one

@levitte
Copy link
Member

levitte commented Mar 20, 2019

I wonder if this is true on all platforms... I think that Windows demands that all symbols be resolved when linking the DLL, and I know for a fact that this is the case on VMS.

I could imagine that being highly platform specific

I believe that -z defs (update: at least with the GNU toolchain) may be the answer... I wonder if we should have something like that for all our modules, engines and providers alike...

crypto/build.info Show resolved Hide resolved
crypto/sha/build.info Outdated Show resolved Hide resolved
3.0 New Core + FIPS automation moved this from In progress to Needs review Mar 21, 2019
@mattcaswell mattcaswell changed the title WIP: Add a FIPS provider and implement SHA256 in it Add a FIPS provider and implement SHA256 in it Mar 21, 2019
@mattcaswell
Copy link
Member Author

I updated this to address comments so far and rebased to pull in the commits from #8513.

I dropped the changes to shlibloadtest because I found it to be unreliable. Instead I've amended the symbol_presence test to check for libcrypto symbols in fips.so that shouldn't be there.

I've also taken this out of WIP.

providers/fips/fipsprov.c Outdated Show resolved Hide resolved
test/evp_extra_test.c Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Mar 21, 2019

We could really make sure that all modules are linked with all symbols resolved. This requires adding this appropriately in Configurations/shared_info.pl:

        dso_ldflags => '-z defs', 

... or similar depending on the platform

It does make sense to do this for engines as well, really, and they should get an additional -lcrypto if they don't already have that

@mattcaswell
Copy link
Member Author

Fixups pushed addressing the feedback so far. The "-z defs" change kind of makes the symbol_presence checks redundant...but that only impacts linux so maybe its useful still on some platforms? I've left it in for now.

@levitte
Copy link
Member

levitte commented Mar 21, 2019

Fixups pushed addressing the feedback so far. The "-z defs" change kind of makes the symbol_presence checks redundant...but that only impacts linux so maybe its useful still on some platforms? I've left it in for now.

Good idea. That needs a more general fixup for other Unixen, and possibly for Windows

@mattcaswell
Copy link
Member Author

Hmmm....I have no idea what to do about this travis error:

Unknown operating system family darwin

make[1]: *** [providers/fips.ld] Error 255

make: *** [all] Error 2

+//// MAKE FAILED

@mattcaswell
Copy link
Member Author

Ah! This error comes from mkdef.pl:

openssl/util/mkdef.pl

Lines 109 to 143 in b3d113e

my %OS_data = (
solaris => { writer => \&writer_linux,
sort => sorter_linux(),
platforms => { UNIX => 1,
EXPORT_VAR_AS_FUNCTION => 0 } },
linux => 'solaris', # alias
"bsd-gcc" => 'solaris', # alias
aix => { writer => \&writer_aix,
sort => sorter_unix(),
platforms => { UNIX => 1,
EXPORT_VAR_AS_FUNCTION => 0 } },
VMS => { writer => \&writer_VMS,
sort => OpenSSL::Ordinals::by_number(),
platforms => { VMS => 1,
EXPORT_VAR_AS_FUNCTION => 0 } },
vms => 'VMS', # alias
WINDOWS => { writer => \&writer_windows,
sort => OpenSSL::Ordinals::by_name(),
platforms => { WIN32 => 1,
_WIN32 => 1,
EXPORT_VAR_AS_FUNCTION => 1 } },
windows => 'WINDOWS', # alias
WIN32 => 'WINDOWS', # alias
win32 => 'WIN32', # alias
32 => 'WIN32', # alias
NT => 'WIN32', # alias
nt => 'WIN32', # alias
mingw => 'WINDOWS', # alias
);
do {
die "Unknown operating system family $OS\n"
unless exists $OS_data{$OS};
$OS = $OS_data{$OS};
} while(ref($OS) eq '');

And appears to come ultimately from the "shared_target" value in the "Configurations" files. If that's the case it looks like there are quite a number missing from the list in mkdef.pl. Unfortunately I'm not sure what they should be set to for the missing platforms....@levitte?

@levitte
Copy link
Member

levitte commented Mar 22, 2019

Hmmm... mkdef.pl should check if $target{shared_defflag} is defined, and simply do nothing if it isn't (except exit 0). I think...
Remind me on Tuesday if you don't figure it out on your own.

@paulidale
Copy link
Contributor

@levitte late reminder :)

providers/build.info Outdated Show resolved Hide resolved
@mattcaswell
Copy link
Member Author

Travis still failed, although this time only in one build. In that build the symbol_presence test failed - complaining that all libcrypto symbols were absent. I don't see how this PR could have caused that (especially since all the other tests passed successfully) - so I suspect an anomaly. The appveyor failure was an issue and I have pushed a fixup for that. Lets see what happens this time around.

@mattcaswell
Copy link
Member Author

Appveyor passed this time but Travis still failed :-(

I really have no clue as to why the symbol_presence test is failing in this one build.

@levitte - any ideas?

test/recipes/01-test_symbol_presence.t Outdated Show resolved Hide resolved
test/recipes/01-test_symbol_presence.t Outdated Show resolved Hide resolved
@levitte
Copy link
Member

levitte commented Mar 29, 2019

On second thought, the test/recipes/01-test_symbol_presence.t changes aren't terribly useful. After all, the only thing you get to do with that is to check that the fips module exports OSSL_provider_init, and that's terribly exciting... Woohoo. I suspect that other stuff will break if that single symbol isn't exported.

@mattcaswell
Copy link
Member Author

Fix-up commits pushed addressing @levitte's comments.

@mattcaswell
Copy link
Member Author

Grrr...travis failure still. Pushed an update that hopefully fixes it. Also, offline, @levitte said to me that he had changed his mind about where the sources for the module should be specified. I've changed it back to the way I used to have it - which is what I think you intended.

@levitte
Copy link
Member

levitte commented Apr 4, 2019

You understood me perfectly, Matt. The reasoning is that for large number of source files, it's better to keep the specs together, and close to the source, the same way we currently specify what goes into libcrypto. It also permits us to use common lists instead of copying so much, i.e. something like this:

{- our $common_source = join(' ', qw(foo1.c foo2.c)); -}
SOURCE[../.../default]=whatever_def.c {- $common_source -}
SOURCE[../.../fips]={- $common_source -} whatever_fips.c

Something like that...

@mattcaswell
Copy link
Member Author

Including mem_dbg.c turns out to be not so easy...

@levitte
Copy link
Member

levitte commented Apr 4, 2019

Yeah, I've noticed... I was surprised how little you were sucking into the FIPS module! The whole thing is quite entangled.

A solution might be to turn off mdebug when building the FIPS module. It's not like the data collected will do anyone anything good anyway, unless you plan on calling its leak checking function at teardown...

@mattcaswell
Copy link
Member Author

I was surprised how little you were sucking into the FIPS module! The whole thing is quite entangled.

Much more will come in later (I've already have some work to do this) - but I wanted to keep it minimal for this PR

@mattcaswell
Copy link
Member Author

Another attempt at getting travis to go through successfully. I've avoided pulling in mem_dbg.c because that wants to suck in a whole load of dependencies that I want to avoid at this stage. Instead I've just disabled crypto-mdebug within the context of the FIPS module (but not elsewhere) for now. I also found and fixed a mem-leak in the test while doing this!

@mattcaswell
Copy link
Member Author

Woop! All the CIs passed!

3.0 New Core + FIPS automation moved this from Needs review to Reviewer approved Apr 4, 2019
@mattcaswell mattcaswell added the approval: done This pull request has the required number of approvals label Apr 4, 2019
@mattcaswell
Copy link
Member Author

Pushed. Thanks!!!

@mattcaswell mattcaswell closed this Apr 4, 2019
3.0 New Core + FIPS automation moved this from Reviewer approved to Done Apr 4, 2019
levitte pushed a commit that referenced this pull request Apr 4, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #8537)
levitte pushed a commit that referenced this pull request Apr 4, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #8537)
levitte pushed a commit that referenced this pull request Apr 4, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #8537)
levitte pushed a commit that referenced this pull request Apr 4, 2019
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #8537)
@opensignature opensignature mentioned this pull request Apr 15, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants