Mitigate timing leakage in error handling

Bug #1359828 reported by Jason Gerard DeRose
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Dbase32
Fix Released
Critical
Jason Gerard DeRose

Bug Description

In Dbase32 1.1 and earlier, both `db32dec()` and `check_db32()` are susceptible to timing attacks because of how they handle the error condition when the text to be decoded or validated contains invalid Dbase32 characters.

The problem is that both stop at the first base32 block (8 characters) containing an error. Even a fairly crude benchmark makes it clear that you can using timing information to at least know when you've guessed another correct block.

For a somewhat contrived example, say attacker controlled input interacts with secret information in some way such that it should produce valid Dbase32 text when the secret is known. In this scenario, timing information can help you incrementally guess the secret, considerably reducing the effective keyspace of the secret:

$ ./setup.py benchmark
Timing attack test:
   1,933,352: timing_test('aAAAAAAAAAAAAAAAAAAAAAAA')
   1,932,627: timing_test('AAAAAAAAaAAAAAAAAAAAAAAA')
   1,903,255: timing_test('AAAAAAAAAAAAAAAAaAAAAAAA')

Worse, the 1.1 and earlier implementation probably leaks enough information to know when you've guessed another correct character, because when a block contains an error, we then step through the block again to find the first invalid character, which is used in the exception we raise. For example, consider the 1.1 check_db32() C implementation:

   count = txt_len / 8;
    for (block=0; block < count; block++) {
        r = DB32_REVERSE[txt_buf[0]];
        r |= DB32_REVERSE[txt_buf[1]];
        r |= DB32_REVERSE[txt_buf[2]];
        r |= DB32_REVERSE[txt_buf[3]];
        r |= DB32_REVERSE[txt_buf[4]];
        r |= DB32_REVERSE[txt_buf[5]];
        r |= DB32_REVERSE[txt_buf[6]];
        r |= DB32_REVERSE[txt_buf[7]];
        if (r & 224) {
            for (i=0; i < 8; i++) {
                r = DB32_REVERSE[txt_buf[i]];
                if (r & 224) {
                    PyErr_Format(PyExc_ValueError,
                        "invalid Dbase32 letter: %c", txt_buf[i]
                    );
                    return NULL;
                }
            }
            PyErr_SetString(PyExc_RuntimeError, "something went very wrong");
            return NULL;
        }
        txt_buf += 8;
    }

This all came about because the original Dbase32 python reference implementation would raise an exception with the first invalid character, which seemed helpful at the time (and it did help the unit tests be a bit more rigorous).

Note that I'm not aware of any specific scenario in Dmedia in which you could actually exploit this, but this is something we should still nip in the bud regardless.

So for 1.2, I'm fixing this in the C implementation. The Python implementation has similar problems, but I'm less concerned about it because it's really just there to help the correctness of the C implementation, not something I see fit for day to day use.

I think it's good for the Python implementation to be quite different in its approach, because both implementations are subject to the same unit tests and are tested against each other.

Tags: security

Related branches

description: updated
description: updated
summary: - Mitagate timing leakage in error handling
+ Mitigate timing leakage in error handling
Changed in dbase32:
status: In Progress → Fix Committed
Changed in dbase32:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.