On 27 Mar 2019, at 16:49, ganis <[log in to unmask]> wrote:Hi,This looks a bug, indeed.I’ll have a look asap.Thanks for reporting.GerriOn 27 Mar 2019, at 14:09, Albert Rossi <[log in to unmask]> wrote: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-3023From: Albert Rossi
Sent: Wednesday, March 27, 2019 7:56 AM
To: xrootd-dev
Cc: Paul Millar; Dmitry O Litvintsev
Subject: padded vs non-padded keyHi 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).
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 cipherktmp = 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):
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 *);#elsestatic 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#endifHere is what Paul has to say:"They create a stack buffer overflow ... exactly the right number of bytes is allocated off the stack forktmp
, but thenDH_compute_key_padded
issues amemmove
to make space for the padding. This, then, will overwrite whatever is afterktmp
on the stack, possibly the return address for theDH_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):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?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; }
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-3023Use REPLY-ALL to reply to listTo unsubscribe from the XROOTD-DEV list, click the following link:
https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1
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