Oh, sorry, now that I look at the code again, I see that the bug is actually in your code that calls the padded routine, not in the routine itself.
It is the way you allocate space for ktmp. So it looks like it needs fixing in XrootD.
Thanks, Paul, for catching this.
Al
________________________________________________
Albert L. Rossi
Application Developer & Systems Analyst III
Scientific Computing Division, Data Movement Development
FCC 229A
Mail Station 369 (FCC 2W)
Fermi National Accelerator Laboratory
Batavia, IL 60510
(630) 840-3023
________________________________
From: Albert Rossi
Sent: Wednesday, March 27, 2019 7:56 AM
To: xrootd-dev
Cc: Paul Millar; Dmitry O Litvintsev
Subject: padded vs non-padded key
Hi all,
Sorry I have been silent for a while, but I have been chasing down and trying to solve an urgent dCache issue unrelated to XrootD.
I just wanted to get back to you about the padding error.
Let me reiterate that the original error we were experiencing (with pre-4.9 XrootD) we think was provoked by the fact that the generation of the DH session key in XrootD did not use padding, whereas the version of Bouncy Castle we moved to did.
Originally, this seemed to be a question of which openssl method was used for the key finalization (there were two, one padded, one unpadded).
(https://github.com/openssl/openssl/blob/master/crypto/dh/dh_key.c)
[https://avatars0.githubusercontent.com/u/3279138?s=400&v=4]<https://github.com/openssl/openssl/blob/master/crypto/dh/dh_key.c>
openssl/dh_key.c at master · openssl/openssl · GitHub<https://github.com/openssl/openssl/blob/master/crypto/dh/dh_key.c>
github.com
Join GitHub today. GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
We did an experiment: we took a 4.8.4 XrootD server and hacked it to use the padded openssl method. This solved the problem for us.
However, we did not want to ask you to do this because we didn't wish to create an backward-compatibility issue.
So we explored a BouncyCastle solution. It turned out that the Java library built in an attempt at backward compatibility (a "preTLS" version of the algorithm). Using that against the old unpadded routine in Xrootd worked.
Until 4.9.
Looking at the 4.9 code, I see that at XrdCryptosslCipher.cc, line 535ff., you now have a switch.
if (DH_generate_key(fDH)) {
// Now we can compute the cipher
ktmp = new char[DH_size(fDH)];
memset(ktmp, 0, DH_size(fDH));
if (ktmp) {
if (padded) {
ltmp = DH_compute_key_padded((unsigned char *)ktmp,bnpub,fDH);
} else {
ltmp = DH_compute_key((unsigned char *)ktmp,bnpub,fDH);
}
if (ltmp > 0) valid = 1;
}
}
I have not had a chance to look further at your code to see when the 'padded' flag is set, but I imagine this corresponds to whether the version of the GSI is >= 10400 (4.9) ...
I need to retest whether using the regular (rather than 'preTLS') algorithm in BouncyCastle fixes our problem. I have a recollection currently that I tested this a couple of weeks OK, but my memory has become hazy because of the other work I have been involved in, so I won't swear to it.
When I point this out to our team, Paul went ahead and drilled down to look at your padded procedure (I had not yet).
It would seem noticed a bug in that code (I also see this explanation for its presence: https://gitlab.cern.ch/dss/xrootd/commit/7f10d67bff12d9c67681a38f650a3e51b6f11616):
[https://gitlab.cern.ch/assets/gitlab_logo-7ae504fe4f68fdebb3c2034e36621930cd36ea87924c11ff65dbcb8ed50dca58.png]<https://gitlab.cern.ch/dss/xrootd/commit/7f10d67bff12d9c67681a38f650a3e51b6f11616>
xrdcrypto: improve padding support for incomplete openssl versions (7f10d67b) · Commits · dss / xrootd<https://gitlab.cern.ch/dss/xrootd/commit/7f10d67bff12d9c67681a38f650a3e51b6f11616>
gitlab.cern.ch
Default openssl on SLC6 provides the function but not the signature
#if !defined(HAVE_DH_PADDED)
#if defined(HAVE_DH_PADDED_FUNC)
int DH_compute_key_padded(unsigned char *, const BIGNUM *, DH *);
#else
static int DH_compute_key_padded(unsigned char *key, const BIGNUM *pub_key, DH *dh)
{
int rv, pad;
rv = dh->meth->compute_key(key, pub_key, dh);
if (rv <= 0)
return rv;
pad = BN_num_bytes(dh->p) - rv;
if (pad > 0) {
memmove(key + pad, key, rv);
memset(key, 0, pad);
}
return rv + pad;
}
#endif
#endif
Here is what Paul has to say:
"They create a stack buffer overflow ... exactly the right number of bytes is allocated off the stack for ktmp, but then DH_compute_key_padded issues a memmove to make space for the padding. This, then, will overwrite whatever is after ktmp on the stack, possibly the return address for the DH_compute_key_padded call."
However, your code simply lifts the openssl code into the XrootD object code:
(from the above cited library code, dh_key.c):
int DH_compute_key_padded(unsigned char *key, const BIGNUM *pub_key, DH *dh)
{
int rv, pad;
rv = dh->meth->compute_key(key, pub_key, dh);
if (rv <= 0)
return rv;
pad = BN_num_bytes(dh->p) - rv;
if (pad > 0) {
memmove(key + pad, key, rv);
memset(key, 0, pad);
}
return rv + pad;
}
Perhaps you and Paul can talk further about this and open a github ticket for openssl? XrootD might go ahead and fix it in its native code?
When I finish the other dCache problem-solving I am currently working on, I will retest switching to the newer BouncyCastle for version 10400.
Thanks!
Al
________________________________________________
Albert L. Rossi
Application Developer & Systems Analyst III
Scientific Computing Division, Data Movement Development
FCC 229A
Mail Station 369 (FCC 2W)
Fermi National Accelerator Laboratory
Batavia, IL 60510
(630) 840-3023
########################################################################
Use REPLY-ALL to reply to list
To unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1
