From 71e8e1531b4f12c8298b3a5d43afe71c3b783ede Mon Sep 17 00:00:00 2001 From: Gerardo Ganis Date: Tue, 11 Dec 2018 18:54:47 +0100 Subject: [PATCH 1/4] Sign also client DH parameters --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 128 +++++++++++++++++++++++------ 1 file changed, 105 insertions(+), 23 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index 761cd2c2b98..725918a5b7e 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -1415,6 +1415,7 @@ XrdSecCredentials *XrdSecProtocolgsi::getCredentials(XrdSecParameters *parm, // Buffer / Bucket related XrdSutBuffer *bpar = 0; // Global buffer XrdSutBuffer *bmai = 0; // Main buffer + XrdSutBucket *bck = 0; // Generic bucket // // Decode received buffer @@ -1534,12 +1535,42 @@ XrdSecCredentials *XrdSecProtocolgsi::getCredentials(XrdSecParameters *parm, if (!(bpub = sessionKey->Public(lpub))) return ErrC(ei,bpar,bmai,0, kGSErrNoPublic,"session",stepstr); + // - // Add it to the global list - if (bpar->UpdateBucket(bpub,lpub,kXRS_puk) != 0) - return ErrC(ei,bpar,bmai,0, kGSErrAddBucket, - XrdSutBuckStr(kXRS_puk),"global",stepstr); - delete[] bpub; // bpub is being duplicated inside of 'UpdateBucket' + // If server supports decoding of signed DH, do sign them + if (hs->RemVers >= XrdSecgsiVersDHsigned) { + bck = new XrdSutBucket(bpub,lpub,kXRS_cipher); + if (sessionKsig) { + // Encrypt client DH public parameters with client private key + if (sessionKsig->EncryptPrivate(*bck) <= 0) + return ErrC(ei,bpar,bmai,0, kGSErrExportPuK, + "encrypting client DH public parameters",stepstr); + } else { + return ErrC(ei,bpar,bmai,0, kGSErrExportPuK, + "client signing key undefined!",stepstr); + } + // + // Add it to the global list + if (bpar->AddBucket(bck) != 0) + return ErrC(ei,bpar,bmai,0, kGSErrAddBucket, "main",stepstr); + // + // Export client public key + XrdOucString cpub; + if (sessionKsig->ExportPublic(cpub) < 0) + return ErrC(ei,bpar,bmai,0, kGSErrExportPuK, + "exporting client public key",stepstr); + // Add it to the global list + if (bpar->UpdateBucket(cpub.c_str(),cpub.length(),kXRS_puk) != 0) + return ErrC(ei,bpar,bmai,0, kGSErrAddBucket, + XrdSutBuckStr(kXRS_puk),"global",stepstr); + } else { + // + // Add it to the global list + if (bpar->UpdateBucket(bpub,lpub,kXRS_puk) != 0) + return ErrC(ei,bpar,bmai,0, kGSErrAddBucket, + XrdSutBuckStr(kXRS_puk),"global",stepstr); + delete[] bpub; // bpub is being duplicated inside of 'UpdateBucket' + } // // Add the proxy certificate @@ -3213,30 +3244,32 @@ int XrdSecProtocolgsi::ClientDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, return -1; } + // // If client supports decoding of signed DH, do sign them if (hs->RemVers >= XrdSecgsiVersDHsigned) { - // // Encrypt server DH public parameters with server key if (sessionKver->DecryptPublic(*bck) <= 0) { emsg = "decrypting server DH public parameters"; return -1; } - } - else // server doesn't provide signed DH parameter, disable proxy delegation + } else { + // If the server doesn't provide signed DH parameter, disable proxy delegation if (hs->Options & (kOptsFwdPxy | kOptsSigReq)) { hs->Options &= ~(kOptsFwdPxy | kOptsSigReq); - std::cerr <<"secgsi: no signed DH parameters from " << Entity.host - << ". Will not delegate x509 proxy to it\n" <Cipher(0,bck->buffer,bck->size,cip.c_str()))) { - PRINT("could not instantiate session cipher " - "using cipher public info from server"); - emsg = "could not instantiate session cipher "; + PRINT("could not instantiate session cipher " + "using cipher public info from server"); + emsg = "could not instantiate session cipher "; + return -1; } // Deactivate what not needed any longer @@ -3576,8 +3609,50 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, " - using default"); } - // First get the session cipher - if ((bck = br->GetBucket(kXRS_puk))) { + if (hs->RemVers >= XrdSecgsiVersDHsigned) { + // First get the client public key + if (!(bck = br->GetBucket(kXRS_puk))) { + cmsg = "bucket with client public key missing"; + return -1; + } + XrdOucString cpub; + bck->ToString(cpub); + sessionKver = sessionCF->RSA(cpub.c_str(), cpub.length()); + if (!sessionKver || !sessionKver->IsValid()) { + cmsg = "bucket with client public key contains an invalid key"; + return -1; + } + + // Get the client DH parameters + if (!(bck = br->GetBucket(kXRS_cipher))) { + cmsg = "bucket with client DH parameters missing"; + return -1; + } + + // Decrypt client DH public parameters with client key + if (sessionKver->DecryptPublic(*bck) <= 0) { + cmsg = "decrypting client DH public parameters"; + return -1; + } + + } else { + + // Get the client DH parameters + if (!(bck = br->GetBucket(kXRS_puk))) { + cmsg = "bucket with client DH parameters missing"; + return -1; + } + + // If the client doesn't provide signed DH parameter, disable proxy delegation + if ((PxyReqOpts & kOptsSrvReq)) PxyReqOpts &= ~kOptsSrvReq; + if (hs->Options & (kOptsDlgPxy | kOptsSigReq | kOptsFwdPxy)) + hs->Options &= ~(kOptsDlgPxy | kOptsSigReq | kOptsFwdPxy); + PRINT("no signed DH parameters from client:" << Entity.tident << + " : will not delegate x509 proxy to it"); + } + + // Get the session cipher + if (bck) { // // Cleanup SafeDelete(sessionKey); @@ -3601,10 +3676,15 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, hs->Chain = 0; return -1; } - // - // We need it only once - br->Deactivate(kXRS_puk); + } else { + cmsg = "bucket with DH parameters not found or invalid: cannot finalize session cipher"; + return -1; } + // + // We need it only once + if (hs->RemVers >= XrdSecgsiVersDHsigned) br->Deactivate(kXRS_cipher); + br->Deactivate(kXRS_puk); + // // Decrypt the main buffer with the session cipher, if available if (sessionKey) { @@ -3740,11 +3820,13 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, } // - // Extract the client public key - sessionKver = sessionCF->RSA(*(hs->Chain->End()->PKI())); - if (!sessionKver || !sessionKver->IsValid()) { - cmsg = "server certificate contains an invalid key"; - return -1; + // For old clients, extract the client public key from the certificate + if (hs->RemVers < XrdSecgsiVersDHsigned) { + sessionKver = sessionCF->RSA(*(hs->Chain->End()->PKI())); + if (!sessionKver || !sessionKver->IsValid()) { + cmsg = "client certificate contains an invalid key"; + return -1; + } } // Deactivate certificate buffer (*bm)->Deactivate(kXRS_x509); From e7f1897787143370dc5519a998d4a5e131026240 Mon Sep 17 00:00:00 2001 From: Gerardo Ganis Date: Wed, 12 Dec 2018 19:46:20 +0100 Subject: [PATCH 2/4] xrdcrypto: fix crash to an early call to X509_NAME_free --- src/XrdCrypto/XrdCryptosslgsiAux.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/XrdCrypto/XrdCryptosslgsiAux.cc b/src/XrdCrypto/XrdCryptosslgsiAux.cc index 2513f353abe..e0eeec2c4ca 100644 --- a/src/XrdCrypto/XrdCryptosslgsiAux.cc +++ b/src/XrdCrypto/XrdCryptosslgsiAux.cc @@ -381,7 +381,6 @@ int XrdCryptosslX509CreateProxy(const char *fnc, const char *fnk, PRINT("could not set subject name - return"); return -kErrPX_SetAttribute; } - X509_NAME_free(psubj); // // Create the extension CertProxyInfo @@ -479,6 +478,7 @@ int XrdCryptosslX509CreateProxy(const char *fnc, const char *fnk, PRINT("could not set subject name"); return -kErrPX_SetAttribute; } + X509_NAME_free(psubj); // Set issuer name if (X509_set_issuer_name(xPX, X509_get_subject_name(xEEC)) != 1) { From 9004b160fe9f75484c0a9edd4ee767c3df5b43db Mon Sep 17 00:00:00 2001 From: Gerardo Ganis Date: Wed, 12 Dec 2018 23:42:52 +0100 Subject: [PATCH 3/4] secgsi: add check on the client public key Server checks that the client public key from the bucket matches the key from the certificate --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 41 ++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index 725918a5b7e..eab19f21811 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -3609,13 +3609,13 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, " - using default"); } + XrdOucString cpub; if (hs->RemVers >= XrdSecgsiVersDHsigned) { // First get the client public key if (!(bck = br->GetBucket(kXRS_puk))) { cmsg = "bucket with client public key missing"; return -1; } - XrdOucString cpub; bck->ToString(cpub); sessionKver = sessionCF->RSA(cpub.c_str(), cpub.length()); if (!sessionKver || !sessionKver->IsValid()) { @@ -3738,6 +3738,7 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, hs->Chain = 0; return -1; } + // // Finalize chain: get a copy of it (we do not touch the reference) hs->Chain = new X509Chain(hs->Chain); @@ -3772,6 +3773,32 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, return -1; } + // + // Extract the client public key from the certificate + XrdCryptoRSA *ckey = sessionCF->RSA(*(hs->Chain->End()->PKI())); + if (!ckey || !ckey->IsValid()) { + cmsg = "client certificate contains an invalid key"; + return -1; + } + if (hs->RemVers >= XrdSecgsiVersDHsigned) { + // For new clients, make sure it is the same we got from the bucket + XrdOucString cpubcert; + if ((ckey->ExportPublic(cpubcert) < 0)) { + cmsg = "exporting client public key"; + return -1; + } + if (cpubcert != cpub) { + cmsg = "client public key does not match the one from the bucket!"; + return -1; + } + } else { + // For old clients, set the client public key from the certificate + sessionKver = ckey; + } + + // Deactivate certificate buffer + (*bm)->Deactivate(kXRS_x509); + // // Check if there will be delegated proxies; these can be through // normal request+signature, or just forwarded by the client. @@ -3819,18 +3846,6 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, } } - // - // For old clients, extract the client public key from the certificate - if (hs->RemVers < XrdSecgsiVersDHsigned) { - sessionKver = sessionCF->RSA(*(hs->Chain->End()->PKI())); - if (!sessionKver || !sessionKver->IsValid()) { - cmsg = "client certificate contains an invalid key"; - return -1; - } - } - // Deactivate certificate buffer - (*bm)->Deactivate(kXRS_x509); - // // Extract the MD algorithm chosen by the client String md = ""; From a6983b173a469187ce8ee208775e5a9b8456b58a Mon Sep 17 00:00:00 2001 From: Gerardo Ganis Date: Wed, 12 Dec 2018 23:57:33 +0100 Subject: [PATCH 4/4] secgsi: fine-tuned notification of delegation ignoring Only notify about delegation options being ignored if they were actually asked for. --- src/XrdSecgsi/XrdSecProtocolgsi.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/XrdSecgsi/XrdSecProtocolgsi.cc b/src/XrdSecgsi/XrdSecProtocolgsi.cc index eab19f21811..21d80827fb3 100644 --- a/src/XrdSecgsi/XrdSecProtocolgsi.cc +++ b/src/XrdSecgsi/XrdSecProtocolgsi.cc @@ -3644,11 +3644,13 @@ int XrdSecProtocolgsi::ServerDoCert(XrdSutBuffer *br, XrdSutBuffer **bm, } // If the client doesn't provide signed DH parameter, disable proxy delegation + if ((PxyReqOpts & kOptsSrvReq) || + hs->Options & (kOptsDlgPxy | kOptsSigReq | kOptsFwdPxy)) + PRINT("no signed DH parameters from client:" << Entity.tident << + " : will not delegate x509 proxy to it"); if ((PxyReqOpts & kOptsSrvReq)) PxyReqOpts &= ~kOptsSrvReq; if (hs->Options & (kOptsDlgPxy | kOptsSigReq | kOptsFwdPxy)) hs->Options &= ~(kOptsDlgPxy | kOptsSigReq | kOptsFwdPxy); - PRINT("no signed DH parameters from client:" << Entity.tident << - " : will not delegate x509 proxy to it"); } // Get the session cipher