oss-fuzz noted this issue:
https://oss-fuzz.com/testcase-detail/5363002606419968
Which reports a heap buffer overflow during ossl_method_cache_flush_some
Its occuring because we delete items from the hash table while inside
its doall iterator
The iterator in lhash.c does a reverse traversal of all buckets in the
hash table, and at some point a removal during an iteration leads to the
hash table shrinking, by calling contract. When that happens, the
bucket index becomes no longer valid, and if the index we are on is
large, it exceeds the length of the list, leading to an out of band
reference, and the heap buffer overflow report.
Fix it by preventing contractions from happening during the iteration,
but setting the down_load factor to 0, and restoring it to its initial
value after the iteration is done
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/24867)
Theres a data race between ossl_method_store_insert and
ossl_method_store_do_all, as the latter doesn't take the property lock
before iterating.
However, we can't lock in do_all, as the call stack in several cases
later attempts to take the write lock.
The choices to fix it are I think:
1) add an argument to indicate to ossl_method_store_do_all weather to
take the read or write lock when doing iterations, and add an
is_locked api to the ossl_property_[read|write] lock family so that
subsequent callers can determine if they need to take a lock or not
2) Clone the algs sparse array in ossl_method_store_do_all and use the
clone to iterate with no lock held, ensuring that updates to the
parent copy of the sparse array are left untoucheTheres a data race
between ossl_method_store_insert and ossl_method_store_do_all, as the
latter doesn't take the property lock before iterating.
I think method (2), while being a bit more expensive, is probably the
far less invasive way to go here
Fixes#24672
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/24782)
Reviewed-by: Neil Horman <nhorman@openssl.org>
Release: yes
(cherry picked from commit 0ce7d1f355)
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/24034)
Test the next arithmetic operation to safely determine if adding the
next digit in the passed property string will overflow
Also, noted a bug in the parse_hex code. When parsing non-digit
characters (i.e. a-f and A-F), we do a tolower conversion (which is
fine), and then subtract 'a' to get the hex value from the ascii (which
is definately wrong). We should subtract 'W' to convert tolower
converted hex digits in the range a-f to their hex value counterparts
Add tests to test_property_parse_error to ensure overflow checks work
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Tom Cosgrove <tom.cosgrove@arm.com>
(Merged from https://github.com/openssl/openssl/pull/22874)
ossl_property_list_to_string() didn't quote strings correctly which
could result in a generated property string being unparsable.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/22182)
There is no point in calling OPENSSL_init_crypto() unless we are actually
going to be using the default libctx.
Fixes#20315
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20341)
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/20023)
Fixes#18631
The store lock does not prevent concurrent access to the
property cache, because there are multiple stores.
We drop the newly created entry and use the exisiting one
if there is one already.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19762)
If there is no timer available to seed the stochastic flushing, revert to a
global seed that gets updated each flush.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18906)
Introducing the concept of reserving the store where a number of
provided operation methods are to be stored.
This avoids racing when constructing provided methods, which is
especially pertinent when multiple threads are trying to fetch the
same method, or even any implementation for the same given operation
type.
This introduces a |biglock| in OSSL_METHOD_STORE, which is separate
from the |lock| which is used for more internal and finer grained
locking.
Fixes#18152
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18153)
This fixes a warning from latest gcc.
There is no point in using strncpy here as we
intentionally copy only the string contents without
the terminating NUL. The len is set from strlen().
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/18627)
When adding a property definition cache entry for a given property query
string we add an assert that we are not replacing an existing entry. If we
are then that indicates a bug in the caller.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18458)
The `alg_cleanup` didn't properly clear the OPENSSL_SA leaf that it
had just freed the contents of. Fortunately, `ossl_sa_ALGORITHM_doall_arg()`
allows us to pass the store pointer itself as an extra argument, which
allows a modified `alg_cleanup` to complete the job.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18151)
This adds ossl_method_store_remove_all_provided(), which selectively
removes methods from the given store that are provided by the given
provider.
This also adds the EVP specific evp_method_store_remove_all_provided(),
which matches ossl_method_store_remove_all_provided() but can also
retrieve the correct store to manipulate for EVP functions.
This allows us to modify ossl_provider_self_test() to do the job it's
supposed to do, but through clearly defined functions instead of a
cache flushing call that previously did more than that.
ossl_provider_deactivate() is also modified to remove methods associated
with the deactivated provider, and not just clearing the cache.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18151)
When evp_method_store_flush() flushed the query cache, it also freed
all methods in the EVP method store, through an unfortunate call of
ossl_method_store_flush_cache() with an argument saying that all
methods should indeed be dropped.
To undo some of the confusion, ossl_method_store_flush_cache() is
renamed to ossl_method_store_cache_flush_all(), and limited to do
only that. Some if the items in the internal ALGORITHM structure are
also renamed and commented to clarify what they are for.
Fixes#18150
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18151)
This is a misused function, as it was called during query cache flush,
when the provider operation bits were meant to record if methods for a
certain operation has already been added to the method store.
Fixes#18150
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18151)
This refactors OSSL_LIB_CTX to avoid using CRYPTO_EX_DATA. The assorted
objects to be managed by OSSL_LIB_CTX are hardcoded and are initialized
eagerly rather than lazily, which avoids the need for locking on access
in most cases.
Fixes#17116.
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17881)
Including e_os.h with a path from a header file doesn't work well on
certain exotic platform. It simply fails to build.
Since we don't seem to be able to stop ourselves, the better move is
to move e_os.h to an include directory that's part of the inclusion
path given to the compiler.
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17641)
This takes out the lock step stacks that allow a fast property to name
resolution. Follow on from #17325.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17388)
The existing code does this conversion by searching the hash table for the
appropriate index which is slow and expensive.
Fixes#15867
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/17325)
These global variables were previously overwritten with the same value
every time we created a new OSSL_LIB_CTX. Instead we preinitialise them
with the correct values, and then confirm that settings for each
OSSL_LIB_CTX agree with the preinitialised values.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16980)
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16918)
This mostly entails passing around a provider pointer, and handling
queries that includes a pointer to a provider, where NULL means "any".
This also means that there's a need to pass the provider pointer, not
just down to the cache functions, but also be able to get it from
ossl_method_store_fetch(). To this end, that function's OSSL_PROVIDER
pointer argument is modified to be a pointer reference, so the
function can answer back what provider the method comes from.
Test added.
Fixes#16614
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16725)
Neither queries nor definitions handle duplicated property names well.
Make having such an error.
Fixes#16715
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/16716)
This previously relied on the caller locking the property store correctly.
This is no longer the case so the string database now requires locking.
Fixes#15866
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15871)
It will simply call the given callback for every method found in the
given store.
Fixes#15538Fixes#14837
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15604)
This function searches a property list for a specific property and returns
a pointer to the definition if found. The existing version was O(n) time,
the improved O(log n).
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15614)
This required making some OSSL_PROPERTY types a little less private.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15570)
The new names are ossl_err_load_xxx_strings.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15446)
The int64_t type was converted to int (truncation).
Negative values were not handled at all.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15396)
If the global properties are updated after a provider with a child libctx
has already started we need to make sure those updates are mirrored in
that child.
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15242)
We have the ability to parse a string into a PROPERTY_LIST already. Now
we have the ability to go the other way.
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15242)
User defined property names were not created before the first fetch. The
rationale for this was to only maintain the user names defined by providers.
This was intended to prevent malicious memory use attacks.
Not being able to specify a default query before the first fetch is wrong. This
changes the behaviour of the property query parsing to always create property
names.
Fixes#15218
Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/15222)
Where an object has multiple ex_data associated with it, then we free that
ex_data in order of priority (high priority first).
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/14991)
