[openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
Given OpenSSL's eternal type confusion, this check is meant to trap callers that get an error return (typically -1) from some API returning signed values and pass that on to PACKET_buf_init as a size_t. For example, ssl3_get_message returns a long to signal buffer length, and that makes me nervous. Except, yeah, it relies on undefined behaviour. OTOH as you note we do have a test for this and we've not seen it fail on any compiler. I agree the check is pointless if your sizes are correctly represented as size_t throughout, but perhaps marginally useful for OpenSSL in its current state. I don't feel too strongly about keeping or removing it - what do others think? ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
On 15/10/15 04:11, Pascal Cuoq via RT wrote:
> As of 2015-10-14, the function PACKET_buf_init in ssl/packet_locl.h
> reads:
>
> static inline int PACKET_buf_init(PACKET *pkt, unsigned char *buf,
> size_t len) { /* Sanity check for negative values. */ if (buf + len <
> buf) return 0;
>
> pkt->curr = buf; pkt->remaining = len; return 1; }
>
> Link:
> https://github.com/openssl/openssl/blob/310115448188415e270bb0bef958c7c130939838/ssl/packet_locl.h#L113
>
> The rules of pointer arithmetics are such that the condition (buf +
> len < buf) is always false when it is defined. A modern compiler, or
> even an old compiler, could suppress the entire conditional from the
> generated code without a second thought and without a warning. Please
> read https://lwn.net/Articles/278137/ . Note that in that very
> similar instance, the GCC developers did not acknowledge any bug in
> their compiler, did not change the compiler's behavior, and simply
> regretted that "their project has been singled out in an unfair
> way".
>
> That LWN story is not a about a compiler bug, or in any case, it is
> about a compiler bug that is here to stay. And according to GCC
> developers, to be common to several C compilers.
The LWN story is about secure coding, and a specific pitfall to be aware
of which may compile away a test for buffer underflow. Specifically they
are talking about a length value received from an untrusted source and
testing that it falls within the bounds of a buffer.
That is *not* the case here. This test is not about adding a critical
security check. The contract that PACKET_buf_init provides to code using
it requires that the buffer provided must be (at least) |len| bytes
long. It is for the calling code to perform this necessary security
checks prior to calling PACKET_buf_init. This code can assume that |len|
is from a trusted source.
The purpose of the sanity check is not then for security, but to guard
against programmer error. For a correctly functioning program this test
should never fail. For an incorrectly functioning program it may do. It
is not guaranteed to fail because the test could be compiled away but,
most likely, it will. We can have some degree of confidence that the
test works and does not get compiled away in most instances because, as
you point out, an explicit check for it appears in packettest.c and, to
date, we have had no reported failures.
>
> As far as I can tell, no current compiler recognizes that the very
> same reasoning as in that old LWN story justifies the suppression of
> the conditional. I tested the compilers currently available on
> https://gcc.godbolt.org . However, any compiler willing to miscompile
> the examples in the LWN article would gladly miscompile the function
> PACKET_buf_init given the chance.
>
> If the function is intended to return 0 for large values of len, then
> the test should look like:
>
> if (len > (size_t)(SIZE_MAX / 2)) ...
>
> Here I have chosen a constant that happens to give a behavior close
> to the one obtained by luck with current compilers. If another
> constant makes sense, then that other constant can be used. The
> current implementation is an invitation for the compiler to generate
> code that, even when len is above the limit, sets pkt->curr to buf,
> sets pkt->remaining to len, and returns 1, which is not what is
> intended, and could have serious security-related consequences latter
> on.
The biggest packet that I can think of that we will ever have to deal
with within libssl would be a handshake message. This has a maximum
length of 0xff plus 5 bytes of message header, i.e. 0x104. There
could be an argument to say we should test against this to ensure that
|len| is not too big.
However that does not necessarily guard against the pointer overflowing.
In an incorrect program where len is just a bit bigger than the actual
size of buf this could, theoretically, be sufficient to overflow the
pointer.
>
> If, as the comment implies, the function is not intended to be called
> with a len so large that it causes (uintptr_t)buf + len to be lower
> than (uintptr_t)buf, then please, please, please simplify the
> function by removing this nonsensical "sanity check", and make this
> function, which cannot fail, return void-as the history of the rest
> of OpenSSL shows, remembering of testing all error codes returned by
> called functions is difficult enough, even when only functions that
> have reason to fail return such codes.
You are correct that this has historically been a problem. All the dev
team use the "--strict-warnings" parameter to config. One of the effects
of this is to treat warnings as errors and this will therefore fail on
any function declared with "__owur" where the return value is unused. We
should ensure that PACKET_buf_init is declared with this (and I believe
Emilia is making that change). Therefore this should not be an issue for
this code.
>
> PACKET_buf_init is called with (size_t)-1 for len in
>
[openssl-dev] [openssl.org #3979] New OpenSSL issue: valid certificate fails validation where subject text == issuer text
Fixed now, thanks for the report. Steve. -- Dr Stephen N. Henson. OpenSSL project core developer. Commercial tech support now available see: http://www.openssl.org ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3138] 80-bit Elliptic Curves with !MEDIUM !LOW !EXP cipher list
Curves aren't negotiated with the ciphersuite, but rather via a separate extension. Since OpenSSL 1.0.2, there are SSL_CTX_set1_curves and SSL_CTX_set1_curves_list to configure supported curves: https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_ecdh_auto.html OpenSSL 1.1 also has a security levels API in the works to make this sort of configuration easier. ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3645] openssl-1.0.1h-cmp - Linking issue
openssl-1.0.1h-cmp isn't an official OpenSSL version. You should seek help with whoever provides this library for you. Cheers, Emilia ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3013] Sending SCSV when TLS extensions are disabled
Rejecting - SCSV is not a TLS extension. ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
On 10/15/2015 07:41 AM, Matt Caswell via RT wrote: > > In summary my opinion is: > - I believe the sanity check does have some value in guarding against > programmer error > - If it were to be compiled away this does not have a detrimental impact > on security (it just increases the likelihood of a crash in the event of > a programmer error) Strictly speaking, it is not a matter of "is the check left as-is" vs. "is the check compiled away". C's undefined behavior rules are pretty open-ended, and the compiler is free to generate code such that, if inputs that would trigger that check were supplied, does absolutely anything at all. Absolutely anything at all means just that; it does not need to be limited to the local scope and could include exiting from the program or also reading from /etc/ssh/ssh_host_rsa_key and sending it over the network. Now, the compiler is unlikely to do something "interesting" like that, since it would be at odds with the compiler's goal of producing fast code, but relying on that does not exactly make me comfortable. (N.B. this is not the common case of signed integer overflow that's easy to reason about; pointer arithmetic has its own rules for undefined behavior that get invoked when the resulting pointer would not point to inside (or one past) the same array object that the starting pointer pointed inside. This happens in many, many, many more cases than the current check would catch. Section 6.5.6 of n1256.pdf covers this topic.) -Ben > - There could be a good argument for adding an additional maximum length > check > - I do not believe the function should be made void > ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #3731] BUG darwin FIPS openssl-1.0.2 ssl/t1_lib.c line 472
This was fixed in January: 6fa805f516f5a6ff3872f1d1014a3dc9de460b99 ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
> PACKET_buf_init. This code can assume that |len| is from a trusted source. > > The purpose of the sanity check is not then for security, but to guard against > programmer error. For a correctly functioning program this test should never > fail. I would say that the combination of these two things means that it should be an assert. ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #4095] X509_STORE_get_by_subject crash
This sounds like an application problem. 1) Did you recompile your source? 0.9.7 and 1.0.1 are not binary-compatible. 2) The certificate hash format has changed between 1.0.1 and 0.9.7, which could explain why the lookup no longer works: https://www.openssl.org/docs/manmaster/apps/rehash.html If the above isn't helpful, try asking for help on openssl-users@. Rejecting the ticket (though please reopen if you find new evidence that this is a bug within OpenSSL). Cheers, Emilia ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
[openssl-dev] [openssl.org #4095] X509_STORE_get_by_subject crash
Hi, Recently we updated the openssl crypto from 0.9.7e 25 to 1.0.1e But it is always crashing while vertifying the certificates from image When debugged found that crash is happening when X509_STORE_get_by_subject called with issuer and issuer name is empty X509_STORE_get_by_subject (, X509_LU_CRL, pIssuer, _obj); When searched for pIssuer, using below functions Char * issuer = X509_NAME_oneline (pIssuer, NULL, 0); Issuer is empty(not null). Hence application crashed. Can you please rpovide your inputs why we get the issuer as empty in newer version. But in older version it is correct and non-empty. also why does it crash. It will be helpful if you can provide your inputs for above query. Thanks & regards, Tosif ___ openssl-bugs-mod mailing list openssl-bugs-...@openssl.org https://mta.openssl.org/mailman/listinfo/openssl-bugs-mod___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
On 15/10/15 14:35, Salz, Rich via RT wrote: > >> PACKET_buf_init. This code can assume that |len| is from a trusted source. >> >> The purpose of the sanity check is not then for security, but to guard >> against >> programmer error. For a correctly functioning program this test should never >> fail. > > I would say that the combination of these two things means that it should be > an assert. assert has its uses, but it depends what you are trying to achieve. If you are developing new code and trying to track down a difficult to find problem - very useful. Even potentially if you are trying to track down a difficult issue in a production scenario, you could create a special debug build to help you pinpoint what is going wrong. I'm sure there are other scenarios which would be excellent uses of assert. IMO this test is about protecting ourselves in the unlikely event that a programmer error does not present itself until production. To protect production code assert is useless because it gets compiled away. If we are concerned about potential programmer errors not appearing until we get into production then assert is not the right tool. You could argue that you should use OPENSSL_assert(), but my views on that are well known. OPENSSL_assert calls abort() even in production which is wrong IMO and potentially dangerous. I know opinions differ on that point within the dev team and this ticket is probably not the place to reopen that particular debate. Suffice it to say I would not support the use of OPENSSL_assert here. Matt ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
> PACKET_buf_init. This code can assume that |len| is from a trusted source. > > The purpose of the sanity check is not then for security, but to guard against > programmer error. For a correctly functioning program this test should never > fail. I would say that the combination of these two things means that it should be an assert. ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
On 2015-10-15 15:41, Matt Caswell via RT wrote: > The purpose of the sanity check is not then for security, but to guard > against programmer error. For a correctly functioning program this test > should never fail. For an incorrectly functioning program it may do. It > is not guaranteed to fail because the test could be compiled away but, > most likely, it will. We can have some degree of confidence that the > test works and does not get compiled away in most instances because, as > you point out, an explicit check for it appears in packettest.c and, to > date, we have had no reported failures. What was not entirely clear from the original bug report is that, while the check is not compiled away, it's compiled into something completely different from what is written in the source. Specifically, the check "buf + len < buf" is optimized into "len >> 63" on 64-bit platform, i.e. "(ssize_t)len < 0" or "len > SIZE_MAX / 2". This is not a check for overflow at all, it doesn't even depend on the value of "buf". If this is what was intended then it's better to write it explicitly. If this is not what was intended then some other approach is required. > The biggest packet that I can think of that we will ever have to deal > with within libssl would be a handshake message. This has a maximum > length of 0xff plus 5 bytes of message header, i.e. 0x104. There > could be an argument to say we should test against this to ensure that > |len| is not too big. > > However that does not necessarily guard against the pointer overflowing. > In an incorrect program where len is just a bit bigger than the actual > size of buf this could, theoretically, be sufficient to overflow the > pointer. Right. That's exactly one of the problem with the current code the way it is compiled by optimizing compilers. A couple of other remarks. 1. The mere fact that the (sub)expression "buf + len" is evaluated unconditionally permits a compiler to assume that this is a valid pointer (it's UB otherwise) and hence that it points inside an array. This, in turn, permits the compiler to remove some other checks against "buf + len" or "len" even if they are well-written. I'm not saying that any of current compilers can do it, I'm just saying that undefined behavior is a very delicate thing and it's better not to have it in your program. 2. If you want to simplify checking openssl for undefined behavior it's better to fix even non-critical cases of it. Otherwise everybody have to research it, to maintain their blacklists of things to ignore etc. It's similar to compiler warnings: if you want to get more value from compiler warnings you have to keep your project warnings-free fixing even minor and false ones. HTH -- Alexander Cherepanov ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #4094] Nonsensical pointer comparison in PACKET_buf_init
On 10/15/2015 05:44 AM, Emilia Käsper via RT wrote: > Given OpenSSL's eternal type confusion, this check is meant to trap callers > that get an error return (typically -1) from some API returning signed values > Hmm, do we have a sense for how typically "typically" is? Maybe just adding a check for (len == (size_t)-1) is the right thing to do. -Ben ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3645] openssl-1.0.1h-cmp - Linking issue
Hi, Just as a note, in case anybody would find this thread in search for help in the future: The described issue likely wouldn't have appeared if following the full building instructions (for the CMP patch) as mentioned e.g. in RT#3101: ./config make depend make stacks make Cheers, Martin -Original Message- From: openssl-dev [mailto:openssl-dev-boun...@openssl.org] On Behalf Of EXT Emilia Käsper via RT Sent: Thursday, October 15, 2015 6:22 PM To: ajim_huss...@hotmail.com Cc: openssl-dev@openssl.org Subject: [openssl-dev] [openssl.org #3645] openssl-1.0.1h-cmp - Linking issue openssl-1.0.1h-cmp isn't an official OpenSSL version. You should seek help with whoever provides this library for you. Cheers, Emilia ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev ___ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
