There is one more detail that I forgot to mention and that you may find interesting.
For signed hash verification, I find that with the padded DH key generation, I have to finalize with
AES/CBC/NoPadding
instead of
AES/CBC/PKCS5Padding
The latter continues to work for the messages exchanged during the GSI handshake, but as soon as I start decrypting hashes with it using the already established session key, the cipher needs to be built using the NoPadding spec, or the padding corruption error immediately appears. Switching to NoPadding does the trick (and the results I reported are using that setting).
-Al
Hello Gerri et al,
It seem my memory of intention had substituted itself for a memory of deed. It looks like I did not in fact test switching back to the padded in BC.
As the little table of some quick testing shows, it looks like we are good to go (the two party was a preliminary for compatibility of our server with the 4.8 client).
I have followed protocol (for 10400) and switched off padding when the crypto version says 'sslnopad'.
Thanks for your help,
Al
Hi Gerri,
Yes, I will definitely retest this.
I will look at the documentation to see how to manage the availability (I take it that this will be an option passed to the server).
Thanks for the info,
Al
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) ...
On 28 Mar 2019, at 12:37, Albert Rossi <[log in to unmask]> wrote:
Hi Gerri,
Thanks for investigating. So we are sort of back to square one then.
I'll keep you posted on our end.
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: ganis <[log in to unmask]>
Sent: Thursday, March 28, 2019 5:47 AM
To: Albert Rossi
Cc: xrootd-dev; Paul Millar; Dmitry O Litvintsev
Subject: Re: padded vs non-padded keyHi all,
I had a deeper look at the question and found that there is not problem with this.The allocate space by DH_size is actually the one use to calculate the padding size.In the end the padded function just makes sure that the entire allocated memory is used.No buffer overflow.
Gerri
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.
Gerri
On 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-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 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#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 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-3023
Use 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