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).
| 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.
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).
| 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 forktmp, 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
