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_strcasecmp #18069

Closed
wants to merge 4 commits into from
Closed

OPENSSL_strcasecmp #18069

wants to merge 4 commits into from

Conversation

beldmit
Copy link
Member

@beldmit beldmit commented Apr 8, 2022

This is a WIP PR.

It basically consists of 2 parts:

  • functions ossl_str[n]casecmp based on locale-agnostic POSIX functions
  • global replacement (script to be written)

The problem that I met is segfault in FIPS module because the global loc object isn't available in FIPS. What is a proper way to ensure its per-provider initialization and deinitialization?

TBD: tests

Checklist
  • documentation is added or updated
  • tests are added or updated

include/internal/e_os.h Outdated Show resolved Hide resolved
crypto/ctype.c Outdated Show resolved Hide resolved
@beldmit
Copy link
Member Author

beldmit commented Apr 8, 2022

Segfault in fipsinstall:

LD_LIBRARY_PATH=. apps/openssl fipsinstall -module providers/fips.so -provider_name fips -section_name fips_sect -out test/fipsmodule.cnf
#0  0x00007ffff78b12f8 in __strcasecmp_l_avx () from /lib64/libc.so.6
#1  0x00007ffff7590c8c in ossl_strcasecmp (s1=0x51e770 "CMAC", s2=0x51e8b0 "CMAC") at crypto/ctype.c:300
#2  0x00007ffff758ffa4 in namenum_cmp (a=0x51e750, b=0x7fffffffbe90) at crypto/core_namemap.c:53
#3  0x00007ffff76c2e83 in getrn (lh=0x51db10, data=0x7fffffffbe90, rhash=0x7fffffffbe30) at crypto/lhash/lhash.c:309
#4  0x00007ffff76c291c in OPENSSL_LH_retrieve (lh=0x51db10, data=0x7fffffffbe90) at crypto/lhash/lhash.c:169
#5  0x00007ffff758ff02 in lh_NAMENUM_ENTRY_retrieve (lh=0x51db10, d=0x7fffffffbe90) at crypto/core_namemap.c:27
#6  0x00007ffff75902b4 in namemap_name2num_n (namemap=0x51daa0, name=0x7ffff76d1b15 "CMAC", name_len=4) at crypto/core_namemap.c:179
#7  0x00007ffff759033e in ossl_namemap_name2num_n (namemap=0x51daa0, name=0x7ffff76d1b15 "CMAC", name_len=4) at crypto/core_namemap.c:199
#8  0x00007ffff757a2b6 in put_evp_method_in_store (store=0x0, method=0x51e7b0, prov=0x51e520, names=0x7ffff76d1b15 "CMAC", propdef=0x7ffff76d1000 <FIPS_DEFAULT_PROPERTIES> "provider=fips,fips=yes", data=0x7fffffffc290)
    at crypto/evp/evp_fetch.c:161
#9  0x00007ffff758fcd4 in ossl_method_construct_this (provider=0x51e520, algo=0x7ffff772a200 <fips_macs>, no_store=0, cbdata=0x7fffffffc140) at crypto/core_fetch.c:87
#10 0x00007ffff758f884 in algorithm_do_this (provider=0x51e520, cbdata=0x7fffffffc0a0) at crypto/core_algorithm.c:65
#11 0x00007ffff759c997 in ossl_provider_doall_activated (ctx=0x509510, cb=0x7ffff758f787 <algorithm_do_this>, cbdata=0x7fffffffc0a0) at crypto/provider_core.c:1347
#12 0x00007ffff758f9b7 in ossl_algorithm_do_all (libctx=0x509510, operation_id=3, provider=0x0, pre=0x7ffff758faf1 <ossl_method_construct_precondition>, fn=0x7ffff758fc2c <ossl_method_construct_this>, 
    post=0x7ffff758fb91 <ossl_method_construct_postcondition>, data=0x7fffffffc140) at crypto/core_algorithm.c:109
#13 0x00007ffff758fe21 in ossl_method_construct (libctx=0x509510, operation_id=3, provider_rw=0x7fffffffc1b0, force_store=0, mcm=0x7fffffffc1c0, mcm_data=0x7fffffffc290) at crypto/core_fetch.c:122
#14 0x00007ffff757a751 in inner_evp_generic_fetch (methdata=0x7fffffffc290, prov=0x0, operation_id=3, name_id=0, name=0x7ffff76d2190 "HMAC", properties=0x0, new_method=0x7ffff7584dee <evp_mac_from_algorithm>, 
    up_ref_method=0x7ffff7584c83 <evp_mac_up_ref>, free_method=0x7ffff7584cc4 <evp_mac_free>) at crypto/evp/evp_fetch.c:301
#15 0x00007ffff757a959 in evp_generic_fetch (libctx=0x509510, operation_id=3, name=0x7ffff76d2190 "HMAC", properties=0x0, new_method=0x7ffff7584dee <evp_mac_from_algorithm>, up_ref_method=0x7ffff7584c83 <evp_mac_up_ref>, 
    free_method=0x7ffff7584cc4 <evp_mac_free>) at crypto/evp/evp_fetch.c:353
#16 0x00007ffff7585202 in EVP_MAC_fetch (libctx=0x509510, algorithm=0x7ffff76d2190 "HMAC", properties=0x0) at crypto/evp/mac_meth.c:172
#17 0x00007ffff74dfd9b in verify_integrity (bio=0x51e4a0, read_ex_cb=0x7ffff7a56b37 <ossl_core_bio_read_ex>, 
    expected=0x51e1b0 "\372\071\227\363\355\220\036\216m\r@\266r\006ͱ\360\341\320\317\002Ko\036:\307\032+''", <incomplete sequence \313>, expected_len=32, libctx=0x509510, ev=0x51e0e0, event_type=0x7ffff76d21c1 "Module_Integrity")
    at providers/fips/self_test.c:194
#18 0x00007ffff74e0346 in SELF_TEST_post (st=0x51dd58, on_demand_test=0) at providers/fips/self_test.c:307
#19 0x00007ffff74df57d in OSSL_provider_init_int (handle=0x51ca20, in=0x7ffff7ed5f10 <core_dispatch_+752>, out=0x7fffffffd610, provctx=0x7fffffffd608) at providers/fips/fipsprov.c:696
#20 0x00007ffff74de0e9 in OSSL_provider_init (handle=0x51ca20, in=0x7ffff7ed5c20 <core_dispatch_>, out=0x7fffffffd610, provctx=0x7fffffffd608) at providers/fips/fips_entry.c:18
#21 0x00007ffff7bb9196 in provider_init (prov=0x51ca20) at crypto/provider_core.c:900
#22 0x00007ffff7bb96b7 in provider_activate (prov=0x51ca20, lock=0, upcalls=1) at crypto/provider_core.c:1098
#23 0x00007ffff7bb9902 in ossl_provider_activate (prov=0x51ca20, upcalls=1, aschild=0) at crypto/provider_core.c:1173
#24 0x00007ffff7bb6e9c in provider_conf_activate (libctx=0x0, name=0x51be50 "fips", value=0x51bea0 "fips_sect", path=0x0, soft=0, cnf=0x51b770) at crypto/provider_conf.c:175
#25 0x00007ffff7bb71a7 in provider_conf_load (libctx=0x0, name=0x51be50 "fips", value=0x51bea0 "fips_sect", cnf=0x51b770) at crypto/provider_conf.c:251
#26 0x00007ffff7bb741e in provider_conf_init (md=0x51bfb0, cnf=0x51b770) at crypto/provider_conf.c:307
#27 0x00007ffff7ab38a2 in module_init (pmod=0x51c8d0, name=0x51bd10 "providers", value=0x51bd60 "provider_section", cnf=0x51b770) at crypto/conf/conf_mod.c:375
#28 0x00007ffff7ab33ac in module_run (cnf=0x51b770, name=0x51bd10 "providers", value=0x51bd60 "provider_section", flags=0) at crypto/conf/conf_mod.c:239
#29 0x00007ffff7ab30bb in CONF_modules_load (cnf=0x51b770, appname=0x0, flags=0) at crypto/conf/conf_mod.c:138
#30 0x00000000004405a5 in generate_config_and_load (prov_name=0x7fffffffe16b "fips", section=0x7fffffffe17e "fips_sect", 
    module_mac=0x7fffffffd9d0 "\372\071\227\363\355\220\036\216m\r@\266r\006ͱ\360\341\320\317\002Ko\036:\307\032+''}\313`\332\377\377\377\177", module_mac_len=32, conditional_errors=1, security_checks=1) at apps/fipsinstall.c:205
#31 0x00000000004410ea in fipsinstall_main (argc=9, argv=0x7fffffffdd30) at apps/fipsinstall.c:496
#32 0x000000000044c0f1 in do_cmd (prog=0x505d10, argc=9, argv=0x7fffffffdd30) at apps/openssl.c:418
#33 0x000000000044bd05 in main (argc=9, argv=0x7fffffffdd30) at apps/openssl.c:298

@levitte
Copy link
Member

levitte commented Apr 8, 2022

You might want to breakpoint on ossl_strcasecmp and see what loc is in that call...

@beldmit
Copy link
Member Author

beldmit commented Apr 8, 2022

It is NULL in FIPS provider. What I want is to pass the global object to provider.

@hlandau
Copy link
Member

hlandau commented Apr 8, 2022

Some of the code in crypto/ is duplicated inside the FIPS module, so you have two instances of any global state; one inside the normal libcrypto.so and one inside the FIPS module. It's likely that a separate locale object is being instantiated inside the FIPS module but that the initialization function you've defined isn't being called.

@beldmit
Copy link
Member Author

beldmit commented Apr 8, 2022

7b78463 provided non-null locale_t but I still get segfault in FIPS module. Probably, the ossl_str[n]casecmp should be converted to upcalls both...

crypto/ctype.c Outdated Show resolved Hide resolved
@beldmit
Copy link
Member Author

beldmit commented Apr 8, 2022

@levitte is this a relevant check?

#if (defined(_POSIX_C_SOURCE) && _POSIX_C_SOURCE >= 200809L)

@simo5
Copy link
Contributor

simo5 commented Apr 8, 2022

@beldmit you may want to embedd the locale init function as ca ll into ossl_strcasecmp/ossl_strncasecmp like this:
define static locale_loc = NULL; to insure each instantiation of this variable is NULLed.

Then change the impl:

int ossl_strcasecmp(const char *s1, const char *s2)
{
   int ret;
   if (loc == NULL) {
       ret = ossl_init_casecmp_int();
       /* handle error */
   }
   return strcasecmp_l(s1, s2, loc);
}

This way if you have multiple static loc instantiations you will always be sure loc is autoinitialized at first use.
If you want to handle the case where init fails w/o repeating it you can set loc to a special value (say (-1), and check for it too)

I think the only issue maybe deinitialization, but that should be easy to handle with a library destructor, which should work also when you unload the fips.so module
HTH

@beldmit
Copy link
Member Author

beldmit commented Apr 8, 2022

@simo5 I don't think so, it may cause races so will require locking. One-time initialization looks much better.

@simo5
Copy link
Contributor

simo5 commented Apr 8, 2022

@beldmit you are right, if you can make sure that the initialization is always run when each module that has a copy of the static variable is loaded
Alternatively you need to make sure the symbol is not included in a file that is recompiled into the fips.so binary, but is external to it, and you make the fips module depend on that symbol being exported by libcrypto.so

@beldmit beldmit changed the title WIP: ossl_strcasecmp ossl_strcasecmp Apr 8, 2022
@beldmit beldmit added branch: master Merge to master branch severity: important Important bugs affecting a released version branch: 3.0 Merge to openssl-3.0 branch labels Apr 8, 2022
@beldmit
Copy link
Member Author

beldmit commented Apr 8, 2022

So. This is out of WIP now.

I still have to fix failing tests and writing the documentation, but the code is suitable for review. I'm also going to write 2 tests:

  • ossl_strcasecmp in Turkish locale
  • d2i_X509 && X509_PUBKEY_get0 in Turkish locale.

It's worth testing the proposed solution on Windows in Turkish locale, but I don't have it in my disposal.

I believe after the fix is provided to master, it should be backported to 3.0.

@beldmit
Copy link
Member Author

beldmit commented Apr 9, 2022

In theory on Windows platform _create_locale and _stricmp_l are equivalent to newlocale and strcasecmp_l
_free_locale is equivalent to freelocale

crypto/ctype.c Outdated Show resolved Hide resolved
@beldmit beldmit force-pushed the ossl_strcasecmp branch 3 times, most recently from fbdc7ce to 720b5c6 Compare April 11, 2022 08:44
@t8m
Copy link
Member

t8m commented Apr 11, 2022

Doing it via upcall is probably not a good idea as that means incompatibility of the provider across libcrypto versions.

util/libcrypto.num Outdated Show resolved Hide resolved
providers/fips/fipsprov.c Outdated Show resolved Hide resolved
providers/fips/fipsprov.c Outdated Show resolved Hide resolved
crypto/ctype.c Outdated Show resolved Hide resolved
util/libcrypto.num Outdated Show resolved Hide resolved
@t8m t8m requested review from levitte and paulidale April 19, 2022 15:53
@beldmit
Copy link
Member Author

beldmit commented Apr 21, 2022

Ping for 2nd approve - I think @paulidale 's one needs to be reapproved.

@t-j-h t-j-h added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Apr 21, 2022
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Apr 22, 2022
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Apr 22, 2022
@beldmit
Copy link
Member Author

beldmit commented Apr 22, 2022

Merged. Many thanks!

@beldmit beldmit closed this Apr 22, 2022
openssl-machine pushed a commit that referenced this pull request Apr 22, 2022
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18069)
openssl-machine pushed a commit that referenced this pull request Apr 22, 2022
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18069)
openssl-machine pushed a commit that referenced this pull request Apr 22, 2022
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18069)
openssl-machine pushed a commit that referenced this pull request Apr 22, 2022
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #18069)
@beldmit beldmit deleted the ossl_strcasecmp branch May 18, 2022 15:29
DDvO pushed a commit to mpeylo/cmpossl that referenced this pull request Aug 5, 2022
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from openssl#18069)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources severity: important Important bugs affecting a released version triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants