Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors
On 8/06/2014 11:40 AM, Kurt Roeckx via RT wrote: On Sun, Jun 08, 2014 at 12:01:28AM +0200, Tim Hudson via RT wrote: Already fixed in the 1.0.1 stable branch so it is already included in 1.0.1h onwards and 1.0.1m is the current recommended version. [...] Can you re-run parfait against the current release version of OpenSSL for that branch - i.e. 1.0.1m It seems you have your branches mixed up. The latest version is 1.0.1h. There is an also an 1.0.0m, but that's from an older branch. Opps - you are right - I did indeed mean *1.0.1h* ... 'm' is in the 1.0.0 branch - and I am requesting is for it to be run against the current 1.0.1 version not an older version - which was especially noticeable when it is pointing out an already resolved item. It is always a good idea to run any tools against the current release versions for a particular branch - and also handy to see the same reports against master so that the forward development version also gets items picked up - as it contains the latest not-yet-in-a-release code. For coverity we use master and OpenSSL_1_0_1-stable Tim. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3384] Patch: add ECC strings to ciphers(1), point out difference between DH and ECDH
- Original Message - From: Matt Caswell via RT r...@openssl.org To: hka...@redhat.com Cc: openssl-dev@openssl.org Sent: Monday, June 9, 2014 1:01:05 AM Subject: [openssl.org #3384] Patch: add ECC strings to ciphers(1), point out difference between DH and ECDH * aNULL also includes some SRP based ciphersuites SRP-AES-256-CBC-SHA SSLv3 Kx=SRP Au=None Enc=AES(256) Mac=SHA1 SRP-3DES-EDE-CBC-SHA SSLv3 Kx=SRP Au=None Enc=3DES(168) Mac=SHA1 SRP-AES-128-CBC-SHA SSLv3 Kx=SRP Au=None Enc=AES(128) Mac=SHA1 Thanks, I've missed that. I've added a patch to my branch, but I'd rather not see it merged in master: Inclusion of them in aNULL is quite surprising. SRP cipher suites that do not use certificates are not vulnerable to MitM attack, see section 2.5.2, section 3.1 and 3.3 of rfc5054. In particular: The server MUST send a certificate if it agrees to an SRP cipher suite that requires the server to provide additional authentication in the form of a digital signature. note the phrase additional authentication and: If an attacker learns a user's SRP verifier (e.g., by gaining access to a server's password file), the attacker can masquerade as the real server to that user, and can also attempt a dictionary attack to recover that user's password. That makes it no worse than the PSK key exchange, which is not in the aNULL group. As such, it looks to me as incorrect categorisation of SRP cipher suites. Doubly so, considering that kSRP and SRP return the same set of cipher suites (see the difference between DH and kDH or ECDH and kECDH). * The patch as it is at the moment is only relevant to master. It can't be backported to earlier branches because it includes info on stuff not in those branches. In particular kDHE, DHE, ECDHE, kECHDE etc. If you want to merge something for those branches you might want to provide a second pull. Sure, I'll provide backports for 1.0.1 and 1.0.2. -- Regards, Hubert Kario __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3384] Patch: add ECC strings to ciphers(1), point out difference between DH and ECDH
- Original Message - From: Matt Caswell via RT r...@openssl.org To: hka...@redhat.com Cc: openssl-dev@openssl.org Sent: Monday, June 9, 2014 1:01:05 AM Subject: [openssl.org #3384] Patch: add ECC strings to ciphers(1), point out difference between DH and ECDH * aNULL also includes some SRP based ciphersuites SRP-AES-256-CBC-SHA SSLv3 Kx=SRP Au=None Enc=AES(256) Mac=SHA1 SRP-3DES-EDE-CBC-SHA SSLv3 Kx=SRP Au=None Enc=3DES(168) Mac=SHA1 SRP-AES-128-CBC-SHA SSLv3 Kx=SRP Au=None Enc=AES(128) Mac=SHA1 Thanks, I've missed that. I've added a patch to my branch, but I'd rather not see it merged in master: Inclusion of them in aNULL is quite surprising. SRP cipher suites that do not use certificates are not vulnerable to MitM attack, see section 2.5.2, section 3.1 and 3.3 of rfc5054. In particular: The server MUST send a certificate if it agrees to an SRP cipher suite that requires the server to provide additional authentication in the form of a digital signature. note the phrase additional authentication and: If an attacker learns a user's SRP verifier (e.g., by gaining access to a server's password file), the attacker can masquerade as the real server to that user, and can also attempt a dictionary attack to recover that user's password. That makes it no worse than the PSK key exchange, which is not in the aNULL group. As such, it looks to me as incorrect categorisation of SRP cipher suites. Doubly so, considering that kSRP and SRP return the same set of cipher suites (see the difference between DH and kDH or ECDH and kECDH). * The patch as it is at the moment is only relevant to master. It can't be backported to earlier branches because it includes info on stuff not in those branches. In particular kDHE, DHE, ECDHE, kECHDE etc. If you want to merge something for those branches you might want to provide a second pull. Sure, I'll provide backports for 1.0.1 and 1.0.2. -- Regards, Hubert Kario __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: Locking inefficiency
It has been a while since I looked at this code, and I'd forgotten some of the convolution implicit in the pluggability of the ERR API. Something else for the TODO list I guess. I doubt that anyone is making use of that flexibility, and it would be massively simpler to carve it down to a single compile-time implementation. I think the same is true of the locking code for that matter, and DSO, and ... First, you're right, pthreads_locking_callback() is collapsing everything to a mutex. If you have a patch for it to use pthread_wrlock_*() instead, I'd be interested to see it. Otherwise I could probably hack one together relatively quickly, the real challenge is subjecting it to a workload and getting results - let me know if you would prefer to test a patch I'd cooked up or to roll your own. BTW, I assume that typical pthread implementations, in the fast-path case (read-lock, no contention), require no context-switch? Second, I can't believe that checking for the presence of error state requires a lock, but from your description is sounds like this is the case. And as I dig it looks like this is indeed the case (but only requires a read-lock, hence the crux of your point). Are you able to confirm that crypto/err/err.c::int_thread_get_item() is the hot-spot for this excessive locking? Also, I assume that the workload where you see this is not getting actual errors, right? Ie. it's just frequently checking for errors and there's nothing there, I assume? If so, it seems to me that we should be able to make this nothing-to-report case lockless, at some marginal expense to the slow-path (when there is something to report). But if you tell me that the fast-path of a read-lock essentially does this already, then it would be of questionable value for me to add any avoidance logic to openssl itself. Cheers, Geoff On Fri, Jun 6, 2014 at 9:47 PM, Salz, Rich rs...@akamai.com wrote: A colleague here noticed that the pthreads-based locking loses the distinction between read and write locks. We’ve collected mutex contention data, and found that the CRYPTO_ERR lock, used while getting error info, is one of the biggest offenders. It turns out that pthreads_locking_callback ignores the CRYPTO_READ/WRITE flag that is passed in. It seems fairly simple to update that function to use NPTL rwlock’s. Any interest? We’ll put out a diff and pull request soon. -- Principal Security Engineer Akamai Technologies, Cambridge, MA IM: rs...@jabber.me; Twitter: RichSalz
Query reg multiple CA-Cert in list with same subject
Hi, I have a query for Ca-Cert list. If at gateway we have configured two CA-certs A1 and A2 both having same subject and content except time-stamp of generation. If peer sends Cert matching to A2, gateway tries to validate it with A1(subject being same and configured first in list) and validation fails. 1. is there a way to avoid addition of cert in store if subject and all contents are same except timestamp generation. 2. Or if not 1st, is there way to validate incoming cert with both cert configured in store. Thanks Mukesh
[openssl.org #3392] EVP_SignFinal smashes the stack with RSA key. RSA key provides n,e,d,p,q.
EVP_SignFinal smashes the stack with RSA key. RSA key provides
n,e,d,p,q. RSA_check_key OK.
p and q were solved from n,e,d offline because the key check failed without it.
*
(gdb) r
Starting program: /home/jwalton/openssl-test.exe
Signature: 78f2c9af23b9a2a42e3b57dec454fa43ea6627992f48d40a33da6a7c93f98b4
*** stack smashing detected ***: /home/jwalton/openssl-test.exe terminated
Program received signal SIGABRT, Aborted.
0x77156f79 in __GI_raise (sig=sig@entry=6)
at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
56../nptl/sysdeps/unix/sysv/linux/raise.c: No such file or directory.
*
Ubuntu 14.04 x64 fully patched. It should be the security-equivalent
of 1.0.1h. I'm not sure what other improvements Ubuntu has provided.
$ uname -a
Linux ubuntu 3.13.0-29-generic #53-Ubuntu SMP Wed Jun 4 21:00:20 UTC
2014 x86_64 x86_64 x86_64 GNU/Linux
$ openssl version
OpenSSL 1.0.1f 6 Jan 2014
// g++ -Wall -Wextra -Wconversion -g3 -std=c++11 openssl-test.cpp -o openssl-test.exe -lssl -lcrypto
#include iostream
using std::cout;
using std::cerr;
using std::endl;
#include string
using std::string;
#include memory
using std::unique_ptr;
#include openssl/bn.h
#include openssl/rsa.h
#include openssl/evp.h
#include openssl/err.h
const char nz[] =
20446702916744654562596343388758805860065209639960173505037453331270270518732245
08977372301204320323609709562340204469011575537734525469644875960570778896584888
95017468362112062706438336639499925362469853626937363871851454247879222415857219
92924045675229348655595626434390043002821512765630397723028023792577935108185822
75369257422156693093780503115582009714681996492027000881132703628678639279359312
17624250488602118597634417704467037220158572506211078553986931332640811506974231
88751482418465308470313958250757758547155699749157985955379381294962058862159085
915015369381046959790476428631998204940879604226680285601;
const char ez[] = 65537;
const char dz[] =
23583109899396195101799862623499368829246520235662137651186064319555667005065389
11356936879137503597382515919515633242482643314423192704128296593672966061810149
31632061789402182278402640746140338406535182197235078430096761014345948432406842
76746396884059179774424728049430754391920261073195321175575450790865379829879825
2239662669005735571815740349321655325526085965627529169195827622139772389760
13057175483467867984218114225248961766503010944557397801270779301059273764049922
0015083392425914877847840457278246402760955883376511998277062853834711506435
61410605789710883438795588594095047409018233862167884701;
const char pz[] =
15737705590244743839558616502896029191493197327877753279847020015603526753735923
90718294084119093232085749598005372477289597182368848096852332845373492076546615
30801859889389455120932077199406250387226339056140578989122526711937239401762061
949364440402067108084155200696015505170135950332209194782224750221639;
const char qz[] =
12992175256740635899099334754006444501823007340248226099417932857332386190837921
12746269565434716649972371852989646481333243433270528522640603220881224011247812
49085873464824282666514908127141915943024862618996371026577302203267804867959037
802770797169483022132210859867700312376409633383772189122488119155159;
using BN_ptr = std::unique_ptrBIGNUM, decltype(::BN_free);
using RSA_ptr = std::unique_ptrRSA, decltype(::RSA_free);
using EVP_PKEY_ptr = std::unique_ptrEVP_PKEY, decltype(::EVP_PKEY_free);
using EVP_MD_CTX_ptr = std::unique_ptrEVP_MD_CTX, decltype(::EVP_MD_CTX_destroy);
#define UNUSED(x) ((void)x)
int main(int argc, char* argv[])
{
UNUSED(argc); UNUSED(argv);
int rc;
long err;
RSA_ptr rsa(RSA_new(), ::RSA_free);
BIGNUM *n = NULL, *e = NULL, *d = NULL, *p = NULL, *q = NULL;
rc = BN_dec2bn(n, nz);
if(rc == 0 || n == NULL) {
cerr BN_dec2bn failed for n endl;
exit(1);
}
rsa-n = n;
rc = BN_dec2bn(e, ez);
if(rc == 0 || e == NULL) {
cerr BN_dec2bn failed for e endl;
exit(1);
}
rsa-e = e;
rc = BN_dec2bn(d, dz);
if(rc == 0 || d == NULL) {
cerr BN_dec2bn failed for d endl;
exit(1);
}
rsa-d = d;
rc = BN_dec2bn(p, pz);
if(rc == 0 || p == NULL) {
cerr BN_dec2bn failed for p endl;
exit(1);
}
rsa-p = p;
rc = BN_dec2bn(q, qz);
if(rc == 0 || q == NULL) {
cerr BN_dec2bn failed for q endl;
exit(1);
}
rsa-q = q;
rc = RSA_check_key(rsa.get());
err = ERR_get_error();
if(rc != 1) {
cerr RSA_check_key failed, error 0x std::hex err endl;
exit(1);
}
EVP_PKEY_ptr pkey(EVP_PKEY_new(), ::EVP_PKEY_free);
rc = EVP_PKEY_set1_RSA(pkey.get(), rsa.get());
err = ERR_get_error();
if(rc != 1) {
cerr EVP_PKEY_set1_RSA failed, error 0x std::hex err endl;
exit(1);
}
EVP_MD_CTX_ptr ctx(EVP_MD_CTX_create(), ::EVP_MD_CTX_destroy);
EVP_MD_CTX_init(ctx.get());
const EVP_MD* md = EVP_sha256();
rc = EVP_SignInit(ctx.get(), md);
err = ERR_get_error();
if(rc != 1) {
cerr EVP_SignInit_ex failed, error 0x std::hex err endl;
exit(1);
}
const char
[openssl.org #3393] PATCH: EVP_SignInit.pod update
Its not clear that the signature's buffer size, `s`, is not used as an IN parameter. Under the current docs, the only thing stated is at most EVP_PKEY_size(pkey) bytes will be written. Its kind of misleading since it appears EVP_PKEY_size(pkey) WILL be written regardless of the signature's buffer size. The omission means the following will result in a buffer overflow in some instances (which I experienced): EVP_MD_CTX_init(ctx); rc = EVP_SignInit(ctx, EVP_sha256()); ... unsigned char signature[EVP_MAX_MD_SIZE]; unsigned int size = (unsigned int)sizeof(signature); rc = EVP_SignFinal(ctx, signature, size, pkey); ... * diff --git a/doc/crypto/EVP_SignInit.pod b/doc/crypto/EVP_SignInit.pod index 620a623..14ecc77 100644 --- a/doc/crypto/EVP_SignInit.pod +++ b/doc/crypto/EVP_SignInit.pod @@ -30,9 +30,11 @@ signature context Bctx. This function can be called several times on the same Bctx to include additional data. EVP_SignFinal() signs the data in Bctx using the private key Bpkey and -places the signature in Bsig. The number of bytes of data written (i.e. the -length of the signature) will be written to the integer at Bs, at most -EVP_PKEY_size(pkey) bytes will be written. +places the signature in Bsig. Bsig must be at least EVP_PKEY_size(pkey) +bytes in size. Bs is an OUT paramter, and not used as an IN parameter. +The number of bytes of data written (i.e. the length of the signature) +will be written to the integer at Bs, at most EVP_PKEY_size(pkey) bytes +will be written. EVP_SignInit() initializes a signing context Bctx to use the default implementation of digest Btype. diff --git a/doc/crypto/EVP_SignInit.pod b/doc/crypto/EVP_SignInit.pod index 620a623..14ecc77 100644 --- a/doc/crypto/EVP_SignInit.pod +++ b/doc/crypto/EVP_SignInit.pod @@ -30,9 +30,11 @@ signature context Bctx. This function can be called several times on the same Bctx to include additional data. EVP_SignFinal() signs the data in Bctx using the private key Bpkey and -places the signature in Bsig. The number of bytes of data written (i.e. the -length of the signature) will be written to the integer at Bs, at most -EVP_PKEY_size(pkey) bytes will be written. +places the signature in Bsig. Bsig must be at least EVP_PKEY_size(pkey) +bytes in size. Bs is an OUT paramter, and not used as an IN parameter. +The number of bytes of data written (i.e. the length of the signature) +will be written to the integer at Bs, at most EVP_PKEY_size(pkey) bytes +will be written. EVP_SignInit() initializes a signing context Bctx to use the default implementation of digest Btype.
Re: [openssl.org #3381] Typo in macro name for ASN (1.0.1h)
--On Sunday, June 08, 2014 11:57 PM +0200 Matt Caswell via RT r...@openssl.org wrote: Hi Quanah Thanks for the submission. The problem with correcting this is that technically it forms part of the public API (since the macro is defined in asn1.h). I guess there's probably not a huge risk in changing it, as I can't imagine there's too many people relying on that define being there, but then on the other hand as this is just a minor cosmetic change its probably not worth it. I note that this has been spotted before and that the decision then was to just correct the error string itself (and not the macro name) - see commit 2b4ffc6. I think that's a reasonable compromise, so I'll stick with that decision and not make any further corrections. It could be fixed for 1.0.2 however, right? It's reasonable to expect the API to change across major releases. --Quanah -- Quanah Gibson-Mount Server Architect Zimbra, Inc. Zimbra :: the leader in open source messaging and collaboration __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3381] Typo in macro name for ASN (1.0.1h)
--On Sunday, June 08, 2014 11:57 PM +0200 Matt Caswell via RT r...@openssl.org wrote: Hi Quanah Thanks for the submission. The problem with correcting this is that technically it forms part of the public API (since the macro is defined in asn1.h). I guess there's probably not a huge risk in changing it, as I can't imagine there's too many people relying on that define being there, but then on the other hand as this is just a minor cosmetic change its probably not worth it. I note that this has been spotted before and that the decision then was to just correct the error string itself (and not the macro name) - see commit 2b4ffc6. I think that's a reasonable compromise, so I'll stick with that decision and not make any further corrections. It could be fixed for 1.0.2 however, right? It's reasonable to expect the API to change across major releases. --Quanah -- Quanah Gibson-Mount Server Architect Zimbra, Inc. Zimbra :: the leader in open source messaging and collaboration __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [PATCH] x86_64 asm: fix bn_mul_mont on odd-len BNs
Bump. On Fri, Jun 6, 2014 at 2:35 PM, Fedor Indutny fe...@indutny.com wrote: Hello everyone! Discovered this problem while trying to fix https://github.com/joyent/node/issues/7704. Attached is a fix for it. Cheers, Fedor.
Re: [openssl.org #3381] Typo in macro name for ASN (1.0.1h)
On Sun, Jun 08, 2014 at 10:57:57PM +0200, Matt Caswell via RT wrote: Hi Quanah Thanks for the submission. The problem with correcting this is that technically it forms part of the public API (since the macro is defined in asn1.h). I guess there's probably not a huge risk in changing it, as I can't imagine there's too many people relying on that define being there, but then on the other hand as this is just a minor cosmetic change its probably not worth it. Can I suggest that we have both defines, set to the same value, for now and at some point remove the other? Kurt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: Locking inefficiency
Geoffrey Thorpe ge...@geoffthorpe.com: First, you're right, pthreads_locking_callback() is collapsing everything to a mutex. I was well aware of this and thought we did this for compatibility reasons (because I couldn't think of any other reasonable explanation, I guess). If actual read-write locks are just as portable, I think it's a no-brainer that we should switch to them. (After all, our code is already prepared for that, for applications that provide appropriate custom callbacks. It's just the default that falls behind.) For future work, a lock-free approach (using thread-local storage?) certainly might make sense, but switching to read-write locks in the default callbacks should be a tiny change with significant benefits to multithreaded applications. Bodo
Re: [openssl.org #3381] Typo in macro name for ASN (1.0.1h)
On Mon, Jun 09, 2014 at 11:14:54AM -0700, Quanah Gibson-Mount wrote: It could be fixed for 1.0.2 however, right? It's reasonable to expect the API to change across major releases. The 1.0.2 release is NOT a major release. The ABI is supposed to be stable across both patch and micro releases. The next release that can change the ABI is 1.1.0. -- Viktor. __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors
Hi Tim,
Thanks for your feedback!
On 06/ 7/14 03:01 PM, Tim Hudson via RT wrote:
On 7/06/2014 7:10 PM, Jenny Yung via RT wrote:
Hello,
We ran parfait on OpenSSL and found the following errors in openssl-1.0.1g:
1. Error: Uninitialised memory (CWE 456)
Possible access to uninitialised memory 'num'
at line 267 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/evp/bio_b64.c
in function 'b64_read'.
num allocated at line 146.
num uninitialised when ctx-start != 0 at line 221.
Already fixed in the 1.0.1 stable branch so it is already included in
1.0.1h onwards and 1.0.1m is the current recommended version.
commit a41d5174e27c99d1caefd76a8e927c814ede509e
Author: Dr. Stephen Henson st...@openssl.org
Date: Tue May 6 14:07:37 2014 +0100
Initialize num properly.
PR#3289
PR#3345
(cherry picked from commit 3ba1e406c2309adb427ced9815ebf05f5b58d155)
I ran it on 1.0.1h and confirmed that it has been fixed. Thanks.
2. Error: Null pointer dereference (CWE 476)
Read from null pointer rctx
at line 114 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_REQ_CTX_free'.
Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq _bio'.
Constant 'NULL' passed into function OCSP_REQ_CTX_free,
argument rctx, from call at line 498.
Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
This indicates a different issue is present - in that the error handling
path will leak memory.
rctx-iobuf = OPENSSL_malloc(rctx-iobuflen);
if (!rctx-iobuf)
return 0;
So if malloc fails rctx itself isn't freed - so that will leak. That
will need to be looked at too.
see below
3. Error: Null pointer dereference (CWE 476)
Read from null pointer rctx
at line 268 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_sendreq_nbio'.
Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq_bio'.
Constant 'NULL' passed into function OCSP_sendreq_nbio,
argument rctx, from call at line 495.
Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
Looks good - but missed other issue with memory leak on malloc failure.
4. Error: Null pointer dereference (CWE 476)
Read from null pointer frag
at line 1175 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/ssl/d1_both.c in
function 'dtls1_buffer_message'.
Function dtls1_hm_fragment_new may return constant 'NULL' at
line 189, called at line 1173.
Null pointer introduced at line 189 in function
'dtls1_hm_fragment_new'.
Looks good.
The following changes fixes the errors:
2 --- openssl-1.0.1g/crypto/evp/bio_b64.c.~1~ Tue Jun 3
14:13:33 2014
3 +++ openssl-1.0.1g/crypto/evp/bio_b64.c Tue Jun 3 14:14:23 2014
4 @@ -143,7 +143,7 @@
5
6 static int b64_read(BIO *b, char *out, int outl)
7 {
8 - int ret=0,i,ii,j,k,x,n,num,ret_code=0;
9 + int ret=0,i,ii,j,k,x,n,num=0,ret_code=0;
10 BIO_B64_CTX *ctx;
11 unsigned char *p,*q;
Already covered in previous commits.
12
13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~Tue Jun 3
14:15:18 2014
14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.cTue Jun 3
14:15:46 2014
15 @@ -490,6 +490,9 @@
16
17 ctx = OCSP_sendreq_new(b, path, req, -1);
18
19 + if (!ctx)
20 + return NULL;
21 +
22 do
23 {
24 rv = OCSP_sendreq_nbio(resp, ctx);
Looks reasonable - although I don't think the spin loop there is
appropriate - basically with no delay, and no select, this will spin on
a non-blocking retry condition (which is meant to make it back to the
caller to enter their event loop. That is a broader issue to look at.
I see -- my change blocks the do-while loop. I'll check this some more.
Do you have any suggestions for now?
25 --- openssl-1.0.1g/ssl/d1_both.c.~1~Tue Jun 3 14:16:25 2014
26 +++ openssl-1.0.1g/ssl/d1_both.cTue Jun 3 14:17:26 2014
27 @@ -1172,6 +1172,8 @@
28
29 frag = dtls1_hm_fragment_new(s-init_num, 0);
30
31 + if (!frag)
32 + return 0;
33 memcpy(frag-fragment, s-init_buf-data, s-init_num);
34
35 if ( is_ccs)
That looks good as a patch.
Can you integrate this into the next release of OpenSSL?
Can you re-run parfait against the current release version of OpenSSL
for that branch - i.e. 1.0.1m
It would also be helpful to see suggested patch as a separate RT issue -
so we can discuss and track them
Re: [openssl.org #3381] Typo in macro name for ASN (1.0.1h)
On 9 June 2014 19:42, Kurt Roeckx via RT r...@openssl.org wrote: On Sun, Jun 08, 2014 at 10:57:57PM +0200, Matt Caswell via RT wrote: Hi Quanah Thanks for the submission. The problem with correcting this is that technically it forms part of the public API (since the macro is defined in asn1.h). I guess there's probably not a huge risk in changing it, as I can't imagine there's too many people relying on that define being there, but then on the other hand as this is just a minor cosmetic change its probably not worth it. Can I suggest that we have both defines, set to the same value, for now and at some point remove the other? Unfortunately the correctly spelled version already exists in the codebase but defined to a *different* number. Matt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors
Thank you, Tim.
2. Error: Null pointer dereference (CWE 476)
Read from null pointer rctx
at line 114 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_REQ_CTX_free'.
Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq _bio'.
Constant 'NULL' passed into function OCSP_REQ_CTX_free,
argument rctx, from call at line 498.
Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
This indicates a different issue is present - in that the error handling
path will leak memory.
rctx-iobuf = OPENSSL_malloc(rctx-iobuflen);
if (!rctx-iobuf)
return 0;
So if malloc fails rctx itself isn't freed - so that will leak. That
will need to be looked at too.
Good point! We'll file a RT to check for the NULL pointer and free the
malloced resources on the error exit (multiple places in the function)
12
13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~Tue Jun 3
14:15:18 2014
14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.cTue Jun 3
14:15:46 2014
15 @@ -490,6 +490,9 @@
16
17 ctx = OCSP_sendreq_new(b, path, req, -1);
18
19 + if (!ctx)
20 + return NULL;
21 +
22 do
23 {
24 rv = OCSP_sendreq_nbio(resp, ctx);
Looks reasonable - although I don't think the spin loop there is
appropriate - basically with no delay, and no select, this will spin on
a non-blocking retry condition (which is meant to make it back to the
caller to enter their event loop. That is a broader issue to look at.
Assuming you are referring to the do-while loop when you said 'spin
loop', that should be looked at separately.
Jenny's suggestion to check the return value of OCSP_sendreq_new()
should be a valid check.
Regards,
-- misaki
__
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors
Thank you, Tim.
2. Error: Null pointer dereference (CWE 476)
Read from null pointer rctx
at line 114 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_REQ_CTX_free'.
Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq _bio'.
Constant 'NULL' passed into function OCSP_REQ_CTX_free,
argument rctx, from call at line 498.
Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
This indicates a different issue is present - in that the error handling
path will leak memory.
rctx-iobuf = OPENSSL_malloc(rctx-iobuflen);
if (!rctx-iobuf)
return 0;
So if malloc fails rctx itself isn't freed - so that will leak. That
will need to be looked at too.
Good point! We'll file a RT to check for the NULL pointer and free the
malloced resources on the error exit (multiple places in the function)
12
13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~Tue Jun 3
14:15:18 2014
14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.cTue Jun 3
14:15:46 2014
15 @@ -490,6 +490,9 @@
16
17 ctx = OCSP_sendreq_new(b, path, req, -1);
18
19 + if (!ctx)
20 + return NULL;
21 +
22 do
23 {
24 rv = OCSP_sendreq_nbio(resp, ctx);
Looks reasonable - although I don't think the spin loop there is
appropriate - basically with no delay, and no select, this will spin on
a non-blocking retry condition (which is meant to make it back to the
caller to enter their event loop. That is a broader issue to look at.
Assuming you are referring to the do-while loop when you said 'spin
loop', that should be looked at separately.
Jenny's suggestion to check the return value of OCSP_sendreq_new()
should be a valid check.
Regards,
-- misaki
__
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
Re: [openssl.org #3387] Bug Report with fixes: null pointer and uninitialised memory errors
Hi Tim,
Thanks for your feedback!
On 06/ 7/14 03:01 PM, Tim Hudson via RT wrote:
On 7/06/2014 7:10 PM, Jenny Yung via RT wrote:
Hello,
We ran parfait on OpenSSL and found the following errors in openssl-1.0.1g:
1. Error: Uninitialised memory (CWE 456)
Possible access to uninitialised memory 'num'
at line 267 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/evp/bio_b64.c
in function 'b64_read'.
num allocated at line 146.
num uninitialised when ctx-start != 0 at line 221.
Already fixed in the 1.0.1 stable branch so it is already included in
1.0.1h onwards and 1.0.1m is the current recommended version.
commit a41d5174e27c99d1caefd76a8e927c814ede509e
Author: Dr. Stephen Henson st...@openssl.org
Date: Tue May 6 14:07:37 2014 +0100
Initialize num properly.
PR#3289
PR#3345
(cherry picked from commit 3ba1e406c2309adb427ced9815ebf05f5b58d155)
I ran it on 1.0.1h and confirmed that it has been fixed. Thanks.
2. Error: Null pointer dereference (CWE 476)
Read from null pointer rctx
at line 114 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_REQ_CTX_free'.
Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq _bio'.
Constant 'NULL' passed into function OCSP_REQ_CTX_free,
argument rctx, from call at line 498.
Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
This indicates a different issue is present - in that the error handling
path will leak memory.
rctx-iobuf = OPENSSL_malloc(rctx-iobuflen);
if (!rctx-iobuf)
return 0;
So if malloc fails rctx itself isn't freed - so that will leak. That
will need to be looked at too.
see below
3. Error: Null pointer dereference (CWE 476)
Read from null pointer rctx
at line 268 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/crypto/ocsp/ocsp_ht.c
in function 'OCSP_sendreq_nbio'.
Function OCSP_sendreq_new may return constant 'NULL' at line
171, called at line 491 in function 'OCSP_sendreq_bio'.
Constant 'NULL' passed into function OCSP_sendreq_nbio,
argument rctx, from call at line 495.
Null pointer introduced at line 171 in function
'OCSP_sendreq_new'.
Looks good - but missed other issue with memory leak on malloc failure.
4. Error: Null pointer dereference (CWE 476)
Read from null pointer frag
at line 1175 of
components/openssl/openssl-1.0.1/build/sparcv9-wanboot/ssl/d1_both.c in
function 'dtls1_buffer_message'.
Function dtls1_hm_fragment_new may return constant 'NULL' at
line 189, called at line 1173.
Null pointer introduced at line 189 in function
'dtls1_hm_fragment_new'.
Looks good.
The following changes fixes the errors:
2 --- openssl-1.0.1g/crypto/evp/bio_b64.c.~1~ Tue Jun 3
14:13:33 2014
3 +++ openssl-1.0.1g/crypto/evp/bio_b64.c Tue Jun 3 14:14:23 2014
4 @@ -143,7 +143,7 @@
5
6 static int b64_read(BIO *b, char *out, int outl)
7 {
8 - int ret=0,i,ii,j,k,x,n,num,ret_code=0;
9 + int ret=0,i,ii,j,k,x,n,num=0,ret_code=0;
10 BIO_B64_CTX *ctx;
11 unsigned char *p,*q;
Already covered in previous commits.
12
13 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~Tue Jun 3
14:15:18 2014
14 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.cTue Jun 3
14:15:46 2014
15 @@ -490,6 +490,9 @@
16
17 ctx = OCSP_sendreq_new(b, path, req, -1);
18
19 + if (!ctx)
20 + return NULL;
21 +
22 do
23 {
24 rv = OCSP_sendreq_nbio(resp, ctx);
Looks reasonable - although I don't think the spin loop there is
appropriate - basically with no delay, and no select, this will spin on
a non-blocking retry condition (which is meant to make it back to the
caller to enter their event loop. That is a broader issue to look at.
I see -- my change blocks the do-while loop. I'll check this some more.
Do you have any suggestions for now?
25 --- openssl-1.0.1g/ssl/d1_both.c.~1~Tue Jun 3 14:16:25 2014
26 +++ openssl-1.0.1g/ssl/d1_both.cTue Jun 3 14:17:26 2014
27 @@ -1172,6 +1172,8 @@
28
29 frag = dtls1_hm_fragment_new(s-init_num, 0);
30
31 + if (!frag)
32 + return 0;
33 memcpy(frag-fragment, s-init_buf-data, s-init_num);
34
35 if ( is_ccs)
That looks good as a patch.
Can you integrate this into the next release of OpenSSL?
Can you re-run parfait against the current release version of OpenSSL
for that branch - i.e. 1.0.1m
It would also be helpful to see suggested patch as a separate RT issue -
so we can discuss and track them individually.
I have submitted another RT issue with the patch (after removing the
first
Re: Locking inefficiency
Hey Bodo, On Mon, Jun 9, 2014 at 3:15 PM, Bodo Moeller bmoel...@acm.org wrote: Geoffrey Thorpe ge...@geoffthorpe.com: First, you're right, pthreads_locking_callback() is collapsing everything to a mutex. I was well aware of this and thought we did this for compatibility reasons (because I couldn't think of any other reasonable explanation, I guess). If actual read-write locks are just as portable, I think it's a no-brainer that we should switch to them. (After all, our code is already prepared for that, for applications that provide appropriate custom callbacks. It's just the default that falls behind.) Well, who knows what the legacy is behind some of the SSLeay-esque assumptions, but I dare say that any pthread implementation that didn't have rw-locks is likely to be relegated to history by now. In any case, I suspect we will soon require that supported platforms be those for which automated builds and tests are running regularly. Before the next stable branch is cut, in any case. So I'm going to propose that we initially put this patch into the development head only, and defer a decision on whether to cherry-pick it into stable branches until that testing is in place. The use of mutex instead of rwclock is not a functional bug, and for this to be a noticeable performance issue (ie. the kind of workload where this matters) is likely to be in situations where downstream patching of the openssl release/checkout is OK. Well, until such time as a cherry-pick-to-stable decision can be made with confidence, in any case. I certainly don't want us to replace the mutex-using code with something that uses mutex *or* rwlock depending on whether OPENSSL_PTHREADS_SUCK is defined or not. BTW, I mention this because NPTL headers apparently cage the rwlock definitions in some #ifdef-ery that I think we want to avoid in the mutex-rwlock changes in openssl. Rather than grappling with the will-some-platform-fail-in-some-subtle-way issues, I prefer that we rely on the short-term arrival of platform coverage/testing to detect the issue if there is one to cater for. What do you think? For future work, a lock-free approach (using thread-local storage?) certainly might make sense, but switching to read-write locks in the default callbacks should be a tiny change with significant benefits to multithreaded applications. Yeah, but a couple of things come to mind. (1) rwlocks (under optimised conditions anyway) seem to be essentially lock free in the fast-path case anyway, ie. for the read-lock/no-contention case, due to futex magic. That means no context-switch (to the kernel or otherwise) in that by-far-the-most-common case. So I think a change to rwlocks is likely to eliminate the observable syscall and contention overheads anyway. (2) that error code is pretty convoluted and is one of my top candidates for getting an axe taken to it, so fiddling with it to get the fast-path lockless (in spite of pthread optimisations) sounds like a bad use of time. Cheers, Geoff
[openssl.org #3394] Suggested patch for null pointer errors
Hi,
This is the follow-up patch suggestion for [openssl.org #3387] Bug
Report with fixes: null pointer and uninitialised memory errors, as
requested.
After running parfait on 1.0.1h, I have removed the first part
(uninitialized memory error.)
This is the patch for the other two files:
2 --- openssl-1.0.1g/crypto/ocsp/ocsp_ht.c.~1~Tue Jun 3 14:15:18 2014
3 +++ openssl-1.0.1g/crypto/ocsp/ocsp_ht.cTue Jun 3 14:15:46 2014
4 @@ -490,6 +490,9 @@
5
6 ctx = OCSP_sendreq_new(b, path, req, -1);
7
8 + if (!ctx)
9 + return NULL;
10 +
11 do
12 {
13 rv = OCSP_sendreq_nbio(resp, ctx);
14 --- openssl-1.0.1g/ssl/d1_both.c.~1~Tue Jun 3 14:16:25 2014
15 +++ openssl-1.0.1g/ssl/d1_both.cTue Jun 3 14:17:26 2014
16 @@ -1172,6 +1172,8 @@
17
18 frag = dtls1_hm_fragment_new(s-init_num, 0);
19
20 + if (!frag)
21 + return 0;
22 memcpy(frag-fragment, s-init_buf-data, s-init_num);
23
24 if ( is_ccs)
Thanks,
Jenny Yung
Oracle Solaris Security
__
OpenSSL Project http://www.openssl.org
Development Mailing List openssl-dev@openssl.org
Automated List Manager majord...@openssl.org
Crash in openSSL 1.0.1g
Hello Team, We have recently done the upgrade to openSSL version 1.0.1g and facing many crashes in below code path. Crashes are seen consistently. Any pointer on what went wrong will be really helpful. Thanks for your time !! ==Crash stack trace= (lldb) bt * thread #30: tid = 0x6fdcc, 0x97f34a6a libsystem_kernel.dylib`__pthread_kill + 10, stop reason = signal SIGABRT frame #0: 0x97f34a6a libsystem_kernel.dylib`__pthread_kill + 10 frame #1: 0x911a2b2f libsystem_c.dylib`pthread_kill + 101 frame #2: 0x911d95f3 libsystem_c.dylib`__abort + 199 frame #3: 0x911d952c libsystem_c.dylib`abort + 232 frame #4: 0x911c3227 libsystem_c.dylib`szone_error + 443 frame #5: 0x911c4482 libsystem_c.dylib`free_list_checksum_botch + 50 frame #6: 0x911c7a43 libsystem_c.dylib`tiny_malloc_from_free_list + 458 frame #7: 0x911c811a libsystem_c.dylib`szone_malloc_should_clear + 880 frame #8: 0x911bda9e libsystem_c.dylib`szone_malloc + 24 frame #9: 0x911bb5ab libsystem_c.dylib`malloc_zone_malloc + 75 frame #10: 0x911bbfe7 libsystem_c.dylib`malloc + 53 frame #11: 0x0026100d libxxcrypto.dylib`default_malloc_ex + 45 frame #12: 0x0026190f libxxcrypto.dylib`CRYPTO_malloc + 175 frame #13: 0x002766e0 libxxcrypto.dylib`pkey_hmac_init + 48 frame #14: 0x002f4159 libxxcrypto.dylib`int_ctx_new + 601 frame #15: 0x002f460c libxxcrypto.dylib`EVP_PKEY_CTX_new_id + 44 frame #16: 0x002f66cf libxxcrypto.dylib`EVP_PKEY_new_mac_key + 63 frame #17: 0x004a3b07 libxxssl.dylib`tls1_P_hash + 423 frame #18: 0x004a42d2 libxxssl.dylib`tls1_PRF + 770 frame #19: 0x004a6119 libxxssl.dylib`tls1_final_finish_mac + 633 frame #20: 0x00496fea libxxssl.dylib`ssl3_do_change_cipher_spec + 394 frame #21: 0x00496b23 libxxssl.dylib`ssl3_read_bytes + 3347 frame #22: 0x0049829e libxxssl.dylib`ssl3_get_message + 334 frame #23: 0x0049795a libxxssl.dylib`ssl3_get_finished + 90 frame #24: 0x0048700f libxxssl.dylib`ssl3_connect + 3103 frame #25: 0x004b8463 libxxssl.dylib`SSL_connect + 51 frame #26: 0x00031bcf x`boost::asio::ssl::detail::engine::do_connect(this=0xb12c8a54, =0x, =0) + 19 at engine.ipp:272 frame #27: 0x000bee79 x`boost::asio::ssl::detail::engine::perform(this=unavailable, data=unavailable, length=unavailable, ec=unavailable, bytes_transferred=unavailable, op=unavailable)(void*, unsigned long), void*, unsigned long, boost::system::error_code, unsigned long*) + 73 at engine.ipp:215 =End == Thanks Regards, -NK
