openssl-0004 + uwsgi-0001: 2 new defects (500x/249x); all 10 missing whitepaper entries restored; count 590→592
This commit is contained in:
parent
2e4f7807d5
commit
34e8d9212f
18 changed files with 1366 additions and 4 deletions
147
defects/openssl/patch/openssl-0004-store-cert-subjects-lhash.md
Normal file
147
defects/openssl/patch/openssl-0004-store-cert-subjects-lhash.md
Normal file
|
|
@ -0,0 +1,147 @@
|
|||
# openssl-0004: CWE-407 O(N²) CA name dedup in SSL_add_store_cert_subjects_to_stack
|
||||
|
||||
## Severity: MEDIUM
|
||||
|
||||
## Location
|
||||
`ssl/ssl_cert.c` — `add_uris_recursive()` called from `SSL_add_store_cert_subjects_to_stack()`
|
||||
|
||||
## Description
|
||||
|
||||
`SSL_add_store_cert_subjects_to_stack()` loads CA subject names from a URI store
|
||||
(e.g., a PKCS#11 token or directory) and deduplicates them into a
|
||||
`STACK_OF(X509_NAME)`. It calls `add_uris_recursive()` for the actual loading.
|
||||
|
||||
`add_uris_recursive()` contains this pattern:
|
||||
|
||||
```c
|
||||
while (!OSSL_STORE_eof(ctx) && !OSSL_STORE_error(ctx)) {
|
||||
...
|
||||
if (sk_X509_NAME_find(stack, xn) >= 0) { /* O(N) linear scan */
|
||||
/* Duplicate */
|
||||
X509_NAME_free(xn);
|
||||
} else if (!sk_X509_NAME_push(stack, xn)) {
|
||||
...
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`sk_X509_NAME_find()` calls `internal_find()`. When the stack is **unsorted**
|
||||
(which it is here — `add_uris_recursive` never calls `sk_X509_NAME_sort()`),
|
||||
`internal_find()` falls through to a linear scan:
|
||||
|
||||
```c
|
||||
if (!st->sorted) {
|
||||
for (i = 0; i < st->num; i++) {
|
||||
cmp_ret = cmp_with_thunk(st, &data, st->data + i);
|
||||
if (cmp_ret == 0) { ... return i; }
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
```
|
||||
|
||||
Each call is O(N). Loading N certificates from the store requires N such calls,
|
||||
giving **O(N²)** total comparisons.
|
||||
|
||||
By contrast, both `SSL_add_file_cert_subjects_to_stack` and
|
||||
`SSL_add_dir_cert_subjects_to_stack` received an explicit LHASH fix (visible in
|
||||
the same file) that pre-populates an `LHASH_OF(X509_NAME)` for O(1) duplicate
|
||||
detection. `SSL_add_store_cert_subjects_to_stack` was left behind.
|
||||
|
||||
## Complexity Before Fix
|
||||
|
||||
| N (CA certs in store) | Comparisons (O(N²)) |
|
||||
|-----------------------|---------------------|
|
||||
| 100 | ~5,050 |
|
||||
| 500 | ~125,250 |
|
||||
| 1,000 | ~500,500 |
|
||||
| 5,000 | ~12,502,500 |
|
||||
|
||||
Each comparison calls `xname_sk_cmp → X509_NAME_cmp → X509_NAME_cmp_ex`
|
||||
which allocates and DER-encodes the name — making each O(N) scan carry
|
||||
non-trivial constant work beyond the raw count.
|
||||
|
||||
## Fix
|
||||
|
||||
Pass an `LHASH_OF(X509_NAME)` into `add_uris_recursive()` just as
|
||||
`SSL_add_dir_cert_subjects_to_stack` does for the per-file helper, replacing
|
||||
`sk_X509_NAME_find()` with `lh_X509_NAME_retrieve()`.
|
||||
|
||||
```diff
|
||||
--- a/ssl/ssl_cert.c
|
||||
+++ b/ssl/ssl_cert.c
|
||||
@@ -1022,7 +1022,8 @@ static int add_uris_recursive(STACK_OF(X509_NAME) *stack,
|
||||
-static int add_uris_recursive(STACK_OF(X509_NAME) *stack,
|
||||
- const char *uri, int depth)
|
||||
+static int add_uris_recursive(STACK_OF(X509_NAME) *stack,
|
||||
+ LHASH_OF(X509_NAME) *name_hash,
|
||||
+ const char *uri, int depth)
|
||||
{
|
||||
...
|
||||
if (infotype == OSSL_STORE_INFO_NAME) {
|
||||
if (depth > 0)
|
||||
- ok = add_uris_recursive(stack, OSSL_STORE_INFO_get0_NAME(info),
|
||||
- depth - 1);
|
||||
+ ok = add_uris_recursive(stack, name_hash,
|
||||
+ OSSL_STORE_INFO_get0_NAME(info), depth - 1);
|
||||
} else if (infotype == OSSL_STORE_INFO_CERT) {
|
||||
...
|
||||
- if (sk_X509_NAME_find(stack, xn) >= 0) {
|
||||
+ if (lh_X509_NAME_retrieve(name_hash, xn) != NULL) {
|
||||
/* Duplicate. */
|
||||
X509_NAME_free(xn);
|
||||
} else if (!sk_X509_NAME_push(stack, xn)) {
|
||||
...
|
||||
+ } else {
|
||||
+ lh_X509_NAME_insert(name_hash, xn);
|
||||
}
|
||||
}
|
||||
...
|
||||
}
|
||||
|
||||
int SSL_add_store_cert_subjects_to_stack(STACK_OF(X509_NAME) *stack,
|
||||
const char *store)
|
||||
{
|
||||
int (*oldcmp)(const X509_NAME *const *a, const X509_NAME *const *b)
|
||||
= sk_X509_NAME_set_cmp_func(stack, xname_sk_cmp);
|
||||
+ LHASH_OF(X509_NAME) *name_hash = lh_X509_NAME_new(xname_hash, xname_cmp);
|
||||
+ int ret;
|
||||
+ X509_NAME *xn;
|
||||
+ int idx, num;
|
||||
+
|
||||
+ if (name_hash == NULL) {
|
||||
+ (void)sk_X509_NAME_set_cmp_func(stack, oldcmp);
|
||||
+ return 0;
|
||||
+ }
|
||||
+
|
||||
+ /* Pre-populate lhash with names already on the stack */
|
||||
+ num = sk_X509_NAME_num(stack);
|
||||
+ for (idx = 0; idx < num; idx++) {
|
||||
+ xn = sk_X509_NAME_value(stack, idx);
|
||||
+ lh_X509_NAME_insert(name_hash, xn);
|
||||
+ }
|
||||
+
|
||||
- int ret = add_uris_recursive(stack, store, 1);
|
||||
+ ret = add_uris_recursive(stack, name_hash, store, 1);
|
||||
+ lh_X509_NAME_free(name_hash);
|
||||
(void)sk_X509_NAME_set_cmp_func(stack, oldcmp);
|
||||
return ret;
|
||||
}
|
||||
```
|
||||
|
||||
## Complexity After Fix
|
||||
|
||||
O(N) total — each name is hashed once (O(1) amortized) during insertion.
|
||||
`lh_X509_NAME_retrieve` is O(1) amortized.
|
||||
|
||||
## Speedup
|
||||
|
||||
| N | Before (comparisons) | After (hash ops) | Ratio |
|
||||
|-----|---------------------|------------------|-------|
|
||||
| 100 | 5,050 | ~100 | ~50x |
|
||||
| 500 | 125,250 | ~500 | ~250x |
|
||||
| 1000| 500,500 | ~1,000 | ~500x |
|
||||
|
||||
## References
|
||||
- CWE-407: Inefficient Algorithmic Complexity
|
||||
- `ssl/ssl_cert.c` — `SSL_add_dir_cert_subjects_to_stack()` (already fixed, same file)
|
||||
- `crypto/stack/stack.c` — `internal_find()` linear path when `!st->sorted`
|
||||
194
defects/openssl/unit/OpenSslStoreCertSubjectsTest.java
Normal file
194
defects/openssl/unit/OpenSslStoreCertSubjectsTest.java
Normal file
|
|
@ -0,0 +1,194 @@
|
|||
package unit;
|
||||
|
||||
/**
|
||||
* openssl-0004 unit test
|
||||
*
|
||||
* Models the O(N²) CA subject name dedup in add_uris_recursive()
|
||||
* (called from SSL_add_store_cert_subjects_to_stack) and the O(N) fixed
|
||||
* version that uses a hash set.
|
||||
*
|
||||
* The defect: sk_X509_NAME_find() on an unsorted stack is O(N) linear scan.
|
||||
* Loading N certs from a store calls it N times → O(N²).
|
||||
* The existing SSL_add_dir/file functions already use LHASH for O(1) dedup;
|
||||
* add_uris_recursive was left behind.
|
||||
*
|
||||
* Compile: javac -d . OpenSslStoreCertSubjectsTest.java
|
||||
* Run: java unit.OpenSslStoreCertSubjectsTest
|
||||
*/
|
||||
public class OpenSslStoreCertSubjectsTest {
|
||||
|
||||
// ------------------------------------------------------------------ //
|
||||
// DEFECTIVE: O(N²) — linear scan for each new cert (unsorted stack) //
|
||||
// ------------------------------------------------------------------ //
|
||||
|
||||
/** Models an X509_NAME as a unique integer DN string. */
|
||||
static final class Name {
|
||||
final String dn;
|
||||
Name(String dn) { this.dn = dn; }
|
||||
@Override public boolean equals(Object o) {
|
||||
return o instanceof Name && ((Name) o).dn.equals(dn);
|
||||
}
|
||||
@Override public int hashCode() { return dn.hashCode(); }
|
||||
}
|
||||
|
||||
/**
|
||||
* SLOW — simulates add_uris_recursive with sk_X509_NAME_find (linear).
|
||||
* Returns number of comparison operations performed.
|
||||
*/
|
||||
static long defectiveAddToStack(Name[] incoming, java.util.List<Name> stack) {
|
||||
long ops = 0;
|
||||
for (Name xn : incoming) {
|
||||
// sk_X509_NAME_find: linear scan when stack is unsorted
|
||||
boolean found = false;
|
||||
for (Name existing : stack) {
|
||||
ops++;
|
||||
if (existing.equals(xn)) {
|
||||
found = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (!found) {
|
||||
stack.add(xn);
|
||||
}
|
||||
}
|
||||
return ops;
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------ //
|
||||
// FIXED: O(N) — hash set for O(1) dedup (mirrors LHASH_OF fix) //
|
||||
// ------------------------------------------------------------------ //
|
||||
|
||||
/**
|
||||
* FAST — simulates add_uris_recursive with lh_X509_NAME_retrieve (hash).
|
||||
* Returns number of hash operations performed.
|
||||
*/
|
||||
static long fixedAddToStack(Name[] incoming, java.util.List<Name> stack) {
|
||||
long ops = 0;
|
||||
java.util.HashSet<Name> nameHash = new java.util.HashSet<>(stack);
|
||||
ops += stack.size(); // pre-populate cost (O(N_existing))
|
||||
for (Name xn : incoming) {
|
||||
ops++;
|
||||
if (!nameHash.contains(xn)) {
|
||||
stack.add(xn);
|
||||
nameHash.add(xn);
|
||||
}
|
||||
}
|
||||
return ops;
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------ //
|
||||
// Tests //
|
||||
// ------------------------------------------------------------------ //
|
||||
static int pass = 0, fail = 0;
|
||||
|
||||
static void check(String name, boolean cond) {
|
||||
if (cond) {
|
||||
System.out.println(" PASS " + name);
|
||||
pass++;
|
||||
} else {
|
||||
System.out.println(" FAIL " + name);
|
||||
fail++;
|
||||
}
|
||||
}
|
||||
|
||||
public static void main(String[] args) {
|
||||
System.out.println("=== openssl-0004: SSL_add_store_cert_subjects O(N²) defect ===\n");
|
||||
|
||||
// --- Correctness tests ---
|
||||
|
||||
// Test 1: dedup - distinct names all added
|
||||
{
|
||||
Name[] certs = {new Name("CN=A"), new Name("CN=B"), new Name("CN=C")};
|
||||
java.util.List<Name> slow = new java.util.ArrayList<>();
|
||||
java.util.List<Name> fast = new java.util.ArrayList<>();
|
||||
defectiveAddToStack(certs, slow);
|
||||
fixedAddToStack(certs, fast);
|
||||
check("all distinct names added (slow)", slow.size() == 3);
|
||||
check("all distinct names added (fast)", fast.size() == 3);
|
||||
}
|
||||
|
||||
// Test 2: duplicates are suppressed
|
||||
{
|
||||
Name a1 = new Name("CN=A");
|
||||
Name a2 = new Name("CN=A"); // same DN, different object
|
||||
Name b = new Name("CN=B");
|
||||
Name[] certs = {a1, a2, b, a1};
|
||||
java.util.List<Name> slow = new java.util.ArrayList<>();
|
||||
java.util.List<Name> fast = new java.util.ArrayList<>();
|
||||
defectiveAddToStack(certs, slow);
|
||||
fixedAddToStack(certs, fast);
|
||||
check("duplicates suppressed (slow)", slow.size() == 2);
|
||||
check("duplicates suppressed (fast)", fast.size() == 2);
|
||||
}
|
||||
|
||||
// Test 3: pre-existing stack entries are not duplicated
|
||||
{
|
||||
Name pre = new Name("CN=Pre");
|
||||
Name[] certs = {new Name("CN=Pre"), new Name("CN=New")};
|
||||
java.util.List<Name> slow = new java.util.ArrayList<>();
|
||||
java.util.List<Name> fast = new java.util.ArrayList<>();
|
||||
slow.add(pre);
|
||||
fast.add(pre);
|
||||
defectiveAddToStack(certs, slow);
|
||||
fixedAddToStack(certs, fast);
|
||||
check("pre-existing not duplicated (slow)", slow.size() == 2);
|
||||
check("pre-existing not duplicated (fast)", fast.size() == 2);
|
||||
}
|
||||
|
||||
// Test 4: empty store — no names added
|
||||
{
|
||||
Name[] empty = {};
|
||||
java.util.List<Name> slow = new java.util.ArrayList<>();
|
||||
java.util.List<Name> fast = new java.util.ArrayList<>();
|
||||
defectiveAddToStack(empty, slow);
|
||||
fixedAddToStack(empty, fast);
|
||||
check("empty store → empty stack (slow)", slow.size() == 0);
|
||||
check("empty store → empty stack (fast)", fast.size() == 0);
|
||||
}
|
||||
|
||||
// Test 5: single cert, no existing stack — added once
|
||||
{
|
||||
Name[] certs = {new Name("CN=Solo")};
|
||||
java.util.List<Name> slow = new java.util.ArrayList<>();
|
||||
java.util.List<Name> fast = new java.util.ArrayList<>();
|
||||
defectiveAddToStack(certs, slow);
|
||||
fixedAddToStack(certs, fast);
|
||||
check("single cert added (slow)", slow.size() == 1);
|
||||
check("single cert added (fast)", fast.size() == 1);
|
||||
}
|
||||
|
||||
// --- Complexity comparison ---
|
||||
System.out.println();
|
||||
|
||||
int[] sizes = {100, 500, 1000};
|
||||
for (int N : sizes) {
|
||||
// N unique certs + N duplicate certs (every cert appears twice)
|
||||
Name[] incoming = new Name[N * 2];
|
||||
for (int i = 0; i < N; i++) {
|
||||
incoming[i] = new Name("CN=Issuer-" + i);
|
||||
incoming[N + i] = new Name("CN=Issuer-" + i); // duplicate
|
||||
}
|
||||
|
||||
java.util.List<Name> slowStack = new java.util.ArrayList<>();
|
||||
java.util.List<Name> fastStack = new java.util.ArrayList<>();
|
||||
|
||||
long slowOps = defectiveAddToStack(incoming, slowStack);
|
||||
long fastOps = fixedAddToStack(incoming, fastStack);
|
||||
|
||||
double ratio = (double) slowOps / Math.max(fastOps, 1);
|
||||
|
||||
System.out.printf(
|
||||
" N=%4d slow=%7d ops fast=%5d ops ratio=%.1fx%n",
|
||||
N, slowOps, fastOps, ratio);
|
||||
|
||||
check("N=" + N + ": both stacks have " + N + " names",
|
||||
slowStack.size() == N && fastStack.size() == N);
|
||||
check("N=" + N + ": slow uses at least 5x more ops than fast",
|
||||
ratio >= 5.0);
|
||||
}
|
||||
|
||||
System.out.println("\n--- " + (pass + fail) + " tests: " + pass
|
||||
+ " passed, " + fail + " failed ---");
|
||||
if (fail > 0) System.exit(1);
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue