Replace non-standard pointer bit-fiddling with standard ANSI C

[aloisklink]

Replaces the non-standard C pointer bit-fiddling in bn_nist.c with the ternary conditional operator. As this is the only place #define PTR_SIZE_INT is used, we can then remove it from crypto/bn/bn_local.h.

Benefits

Converts implementation defined behavior to standard ANSI C

Bit-fiddling pointers is technically implementation defined behavior in the C specification so the following code is not supported in all platforms:

    PTR_SIZE_INT mask;
    void * a, b, c;
    int boolean_flag;

    mask = 0 - boolean_flag;
    /* Not guaranteed to be a valid ptr to a or b on all platforms  */
    a = (void *)
        ((((PTR_SIZE_INT) b & ~mask) | (((PTR_SIZE_INT)) c & mask)));

For example, without these patches, the current code doesn't compile for ARM's experimental Morello platform.

In fact, any integer to pointer casting is implementation defined behavior in the ASNI C specification (see http://port70.net/~nsz/c/c89/c89-draft.html#3.3.4).

This also allows us to remove the #define PTR_SIZE_INT size_t, which is great, as there are a couple of esoteric platforms where size_t != uintptr_t.

Using a ternary conditional operator is supported on all platforms (i.e. a = boolean_flag ? b : c;).

Performance

On most modern compilers/CPUs, this will be faster, since it will get converted to a CMOV instruction. (the compiler doesn't know that the return value from bn_sub_words/bn_add_words is a boolean, so it currently can't optimize this to a CMOV).

Potential downsides

In some cases (e.g. older compilers or simpler CPUs), this code will get compiled into branching code. Most simpler CPUs usually have short pipelines, so even if there is a branch misprediction, this shouldn't have too much of a performance impact.

I don't think this will result in any potential timing attacks, since these functions already have a bunch of if() conditionals.

(If there is a risk of timing attacks, we should probably instead make some new functions in include/internal/constant_time.h called constant_time_select_ptr and constant_time_select_func_ptr (since in C, function pointers aren't necessarily the same size as object pointers).)

---

Checklist

There's not an easy way to test for this implementation defined behavior, other than to add a CI job that compiles for a platform that doesn't support bit-fiddling pointers, but that seems like a lot of wasted computation for a platform that very few people will use.

[aloisklink]

I've signed and submitted the CLA, but since I just did it today, I'm guessing it will take some time to process.

[levitte]

Question to @openssl/otc: cherry-pick to 3.1? 3.0? This is both a bug fix for a certain platform and a cleanup

[paulidale]

I'm okay with 3.0 and 3.1 too.

It looks like this original code was intended to be constant time, but the if statements certainly aren't & look more expensive (at least in some instances).

[aloisklink]

Thanks for the reviews!

Closing and re-opening my PR to remove the "cla required" label.

This is both a bug fix for a certain platform

FYI, in case this affects the decision to back-port (or to make a CHANGES.md entry), there's only a few hundred ARM Morello boards in existence, mostly only used for research purposes, so it's currently quite a niche platform.

[tom-cosgrove-arm]

The bit manipulation looks like it is done for constant-time. Replacing it with ?: is likely to lose these constant-time properties - is that acceptable?

[paulidale]

The code surrounding this definitely isn't constant time, so I think it's okay that this isn't.

[davidben]

This file looks to be implementing field arithmetic for ECC. That is typically something that does want to be constant-time. (Of course, how far OpenSSL actually was from achieving constant-time, I can't say. That'd require a more thorough audit than I have time for.)

Although both the old and new code seems off. Historically, OpenSSL would indeed do this sort of constant-time selecting of a pointer, but our understanding of constant-time is much better now. Now we know to keep the entire trace of memory accesses secret-independent. E.g. CVE-2016-0702 and changes like #6141.

It seems this file is still the old style, but I don't think introducing a ternary op is correct either. Rather, the right answer would be to use a conditional copy. (In BoringSSL, we introduced a bn_reduce_once operation because it turns out a conditional subtract to get from [0, 2M) to [0, M) is very very common.)

[davidben]

I'll add the conditional copy would also avoid pointer bit-fiddling, which is indeed a little perilous. Although I think what this file was previously doing was still valid under PNVI.

[paulidale]

I've created #20761 to revisit this "fix" and to make the rest of the code constant time.

[aloisklink]

Rather, the right answer would be to use a conditional copy

Agreed! Using a conditional copy seems to be the best way to avoid pointer bit-fiddling if we want this to be constant-time.

Getting rid of the function pointer ternary might be a bit more verbose, but should be relatively easy with some conditional subtracts/additions.

---

Although I think what this file was previously doing was still valid under PNVI.

The ARM Morello architecture is a bit unconventional. It's 64-bit (similar to ARM64), but pointers are 128/129-bit as it contains a 64-bit memory address, but also a 64-bit pointer capability that says whether the pointer has read/write/execute permissions, and the upper/lower range of the pointer to avoid buffer overflows.

Could the problem have been fixed by using uintptr_t ?
From @tom-cosgrove-arm in #20748 (comment) (replying here since it's related to the above paragraph).

Casting from a pointer to uintptr_t and back still works, but since you can only change the address part of the pointer, doing bit-fiddling like this doesn't work (i.e. if you do void * a = (((uintptr_t) b & ~mask) | ((uintptr_t) c & mask));, the compiler doesn't know whether pointer a should have the permissions of b or c.¹²

Footnotes

1. For more info, see 4.2.3 Single-origin provenance of https://www.cl.cam.ac.uk/techreports/UCAM-CL-TR-947.pdf ↩

2. Compiler Explorer link here, in case you want to see the compiler warnings ↩

[tom-cosgrove-arm]

Using a conditional copy seems to be the best way to avoid pointer bit-fiddling if we want this to be constant-time.

"Conditional" is pretty much the opposite of constant-time, in general. Some compilers, for some systems, will emit code for ?: that operates in constant time, but in general that's not the case.

you can only change the address part of the pointer

Yes, from what I understand uintptr_t in Morello does not behave as a standard integer type under arithmetic and bit-wise operations (although as you say, casting to uintptr_t and back works as expected).

I will note that the C99 standard isn't clear about the limits of casting a pointer to uintptr_t and back. It says:

The following type designates an unsigned integer type with the property that any valid pointer to void can be converted to this type, then converted back to pointer to void, and the result will compare equal to the original pointer.

So all that's guaranteed is testing equal, but given two pointers p and q, where p == q, most developers would expect that p and q can be used interchangeably, so we are in "language lawyer" territory here

[aloisklink]

"Conditional" is pretty much the opposite of constant-time, in general.

I think by conditional copy, @davidben meant something like constant_time_cond_swap_buff()

openssl/include/internal/constant_time.h

Lines 361 to 376 in 8b6bbca

	/*
	* mask must be 0xFF or 0x00.
	* "constant time" is per len.
	*
	* if (mask) {
	* unsigned char tmp[len];
	*
	* memcpy(tmp, a, len);
	* memcpy(a, b);
	* memcpy(b, tmp);
	* }
	*/
	static ossl_inline void constant_time_cond_swap_buff(unsigned char mask,
	unsigned char *a,
	unsigned char *b,
	size_t len)

, which should in theory be a conditional copy in constant time.

So all that's guaranteed is testing equal

I think a language lawyer could go even further and say that's assuming you haven't modified the integer (even if you modify it to something that has the same arithmetic value)!

The uintptr_t conversion is also only guaranteed to work for object pointers (pointer to void), (although even on Morello, function pointers and object pointers are the same).

As far as I'm aware, having different sizes for function and object pointers is usually only tied to 16-bit architectures like MS-DOS, but I guess anybody doing crypto on tiny microprocessors would use something like wolfssl instead :)

[paulidale]

Several architectures have different pointers for the two not just old segmented x86. I can't think of any modern processors that do however (well PIC but I doubt OpenSSL fits on any PIC).

That exchange loop looks like a reasonable approach.

[tom-cosgrove-arm]

@aloisklink Could the problem have been fixed by using uintptr_t ?

[beldmit]

I don't like the idea to add non-constant time operations in BIGNUM code, speaking frankly.

[paulidale]

This gem comes just prior to the first non-constant time change:

    if (carry > 0)
        carry =
            (int)bn_sub_words(r_d, r_d, _nist_p_192[carry - 1],
                              BN_NIST_192_TOP);
    else
        carry = 1;

I think the ?: operator will be closer to constant time than this 😁

Some of the other changes aren't so clear cut though.

[openssl-machine]

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

[paulidale]

Merged, thanks for the contribution.