Rework cipher / digest fetching for legacy nids with multiple name support by levitte · Pull Request #9969 · openssl/openssl

levitte · 2019-09-21T19:09:17Z

With multiple names, it's no longer viable to just grab the "first" in
the set and use that to find the legacy NID. Instead, all names for
an algorithm must be checked, and if we encounter more than one NID
asssociated with those names, we consider it an error and make that
method unloadable.

This ensures that all methods that do have an internal NID associated
will get that NID in their structure, thereby ensuring that other
parts of libcrypto that haven't gone away from using NIDs for
comparison will continue to work as expected.

…pport With multiple names, it's no longer viable to just grab the "first" in the set and use that to find the legacy NID. Instead, all names for an algorithm must be checked, and if we encounter more than one NID asssociated with those names, we consider it an error and make that method unloadable. This ensures that all methods that do have an internal NID associated will get that NID in their structure, thereby ensuring that other parts of libcrypto that haven't gone away from using NIDs for comparison will continue to work as expected.

levitte · 2019-09-21T19:10:09Z

Without this, any provider based algorithm implementation with multiple names risk not being detected the way they should, causing all sorts of test failures.

levitte · 2019-09-21T19:11:10Z

Incidently, #8984 and #8985 cannot work without this change (#8985 already has it integrated, but that's ok)

paulidale · 2019-09-22T03:41:37Z

I think this would this be better done as a check when registering names/aliases. The registration happens once, the lookups could be frequent. Failing at registration time would prevent conflicts being introduced, rather than detecting them afterwards.

levitte · 2019-09-22T04:33:18Z

So what you're saying is that you'd like to have a name number <=> legacy NID map, is that right? I did contemplate that, and sure, that can be done as well.

levitte · 2019-09-22T05:14:47Z

Wait, hold on.... This code is executed as part of constructing a method, i.e. during registration already. What I'll end up doing is moving essentially the same code to another spot, but it's still going to be called as part of the construction process.

paulidale · 2019-09-22T05:24:43Z

The function evp_md_from_dispatch is called from EVP_MD_fetch. The former calls evp_doall_names. EVP_MD_fetch isn't part of the registration process. It looks like there is a name search on every fetch. I could easily have missed something...

I'm not suggesting a name <-> NID mapping. We're moving away from NIDs. I'm suggesting that identical names with different NIDs be checked for earlier and that fetches won't need to check.

levitte · 2019-09-22T05:43:19Z

The function evp_md_from_dispatch is called from EVP_MD_fetch

You're mixing up method lookup with method construction. EVP_MD_fetch does lookup first, and only construction if lookup gave no result.

I'm suggesting that identical names with different NIDs be checked for earlier and that fetches won't need to check.

When's this "earlier" you're talking about?

levitte · 2019-09-22T06:27:10Z

We're moving away from NIDs.

Yes, and this is temporary code which should be removed as soon as we stop having code like this:

openssl/crypto/evp/evp_lib.c

Lines 27 to 39 in a941054

    
               if (cipher->prov != NULL) { 
        
                   /* 
        
                    * The cipher has come from a provider and won't have the default flags. 
        
                    * Find the implicit form so we can check the flags. 
        
                    * TODO(3.0): This won't work for 3rd party ciphers we know nothing about 
        
                    * We'll need to think of something else for those. 
        
                    */ 
        
                   cipher = EVP_get_cipherbynid(cipher->nid); 
        
                   if (cipher == NULL) { 
        
                       EVPerr(EVP_F_EVP_CIPHER_PARAM_TO_ASN1, ASN1_R_UNSUPPORTED_CIPHER); 
        
                       return -1; 
        
                   } 
        
               }

But in the mean time, we need something that does this mapping, or we see our tests fail all over the place (I've had to chase down some of them when working on #8984 and #8985).
Of course, we could spend time on chasing down code like the above instead. I thought that it was lower priority for now, and that such an effort was seen as a later clean-up. Maybe I misunderstood...

paulidale · 2019-09-22T08:29:51Z

I think I've been confusing myself, fetch is registration time. With the caching, the searching for clashing names is skipped.

levitte · 2019-09-22T08:54:07Z

Thank you

levitte · 2019-09-23T06:35:36Z

With the caching

With the storing, even. Do remember that we have both a store and a cache.

paulidale · 2019-09-23T06:43:00Z

The only concern I can see here is if an algorithm, not known to libcrypto, is registered by two different providers. This seems pretty unlikely.

I think they'd also need to be in different threads to create a new object race condition.

paulidale · 2019-09-23T06:43:54Z

With the storing, even. Do remember that we have both a store and a cache.

I'm starting to lose track of bits of the architecture :)

levitte · 2019-09-23T07:12:35Z

The only concern I can see here is if an algorithm, not known to libcrypto, is registered by two different providers. This seems pretty unlikely.

Haven't we dealt with that possibility already, and dealt with it? They will have to come with properties that somehow make them unique.

…pport With multiple names, it's no longer viable to just grab the "first" in the set and use that to find the legacy NID. Instead, all names for an algorithm must be checked, and if we encounter more than one NID asssociated with those names, we consider it an error and make that method unloadable. This ensures that all methods that do have an internal NID associated will get that NID in their structure, thereby ensuring that other parts of libcrypto that haven't gone away from using NIDs for comparison will continue to work as expected. Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #9969)

levitte · 2019-09-23T07:15:21Z

Merged.

3204083 Rework cipher / digest fetching for legacy nids with multiple name support

levitte added the branch: master Applies to master branch label Sep 21, 2019

This was referenced Sep 21, 2019

Replumbing: pre-populate the EVP namemap with commonly known names #8984

Closed

Replumbing: make it possible for providers to specify multiple names #8985

Closed

paulidale approved these changes Sep 23, 2019

View reviewed changes

levitte closed this Sep 23, 2019

Uh oh!

Conversation

levitte commented Sep 21, 2019

Uh oh!

levitte commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

levitte commented Sep 21, 2019

Uh oh!

paulidale commented Sep 22, 2019

Uh oh!

levitte commented Sep 22, 2019

Uh oh!

levitte commented Sep 22, 2019

Uh oh!

paulidale commented Sep 22, 2019

Uh oh!

levitte commented Sep 22, 2019

Uh oh!

levitte commented Sep 22, 2019

Uh oh!

paulidale commented Sep 22, 2019

Uh oh!

levitte commented Sep 22, 2019

Uh oh!

levitte commented Sep 23, 2019

Uh oh!

paulidale commented Sep 23, 2019

Uh oh!

paulidale commented Sep 23, 2019

Uh oh!

levitte commented Sep 23, 2019

Uh oh!

levitte commented Sep 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

levitte commented Sep 21, 2019 •

edited

Loading