Skip to content

Commit da5b083

Browse files
elbuo8claude
elbuo8
and
claude
authored
fix: okta sibling case (#151)
* fix: resolve XML-Enc elements by DOM traversal instead of XPath decryptKeyInfo located elements with XPath expressions that were built by string concatenation and scoped by a leading "//". Both properties made the lookups behave differently from how they read. "//" is descendant-or-self over the whole document regardless of the context node passed to select(), so lookups that appear scoped were document-wide: - the wrapped key was taken from the first CipherValue anywhere in the document rather than from the EncryptedKey being processed - DigestMethod was resolved by an absolute path, so with several EncryptedKey elements it could return a digest declared by one key while the ciphertext came from another - the absolute DigestMethod path also matched nothing for documents using EncryptedData/KeyInfo/RetrievalMethod, where EncryptedKey sits outside KeyInfo, silently defaulting oaepHash to sha1 Predicates were also assembled by concatenating the RetrievalMethod URI into the expression text, so an Id containing a quote changed what the expression meant instead of being compared as a value. lib/dom-select.js walks the DOM explicitly. Document values are compared, never concatenated into anything that gets parsed, and scope is a node enforced by the walk itself. EncryptionMethod and CipherValue are now read from one resolved EncryptedKey, so they always describe the same key. Two input-validation changes come with this: - a RetrievalMethod URI must be a same-document fragment reference. substring(1) removed the first character whatever it was, so "/ek1" and "Xek1" both resolved to Id "ek1". - a duplicated Id is reported rather than resolved to its first match. Duplicate Ids are invalid XML and resolving them makes the result depend on document order. xpath moves to devDependencies; the runtime no longer parses expressions. Verified with a packed tarball installed via --omit=dev, where require('xpath') throws MODULE_NOT_FOUND and both fixtures still decrypt. 65 passing (44 before). Each property is pinned by a test that also asserts the previous XPath behaviour for contrast, and 11 mutations of the traversal and the guards were each confirmed to fail a named test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: replace leftover xpath calls with dom.child; align OAEP test fixtures with mgf1=sha1 * fix: okta sibling case --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent c9c4de5 commit da5b083

4 files changed

Lines changed: 147 additions & 5 deletions

File tree

‎lib/xmlenc.js‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -254,14 +254,16 @@ function decrypt(xml, options, callback) {
254254
var doc = typeof xml === 'string' ? new xmldom.DOMParser().parseFromString(xml) : xml;
255255

256256
var symmetricKey = decryptKeyInfo(doc, options);
257-
var encryptionMethod = dom.byPath(doc, ['EncryptedData', 'EncryptionMethod']);
257+
// Same EncryptedData that decryptKeyInfo resolved the key for.
258+
var encryptedData = dom.byPath(doc, ['EncryptedData']);
259+
var encryptionMethod = dom.child(encryptedData, 'EncryptionMethod');
258260
var encryptionAlgorithm = encryptionMethod.getAttribute('Algorithm');
259261

260262
if (options.disallowDecryptionWithInsecureAlgorithm !== false
261263
&& insecureAlgorithms.indexOf(encryptionAlgorithm) >= 0) {
262264
return callback(new Error('encryption algorithm ' + encryptionAlgorithm + ' is not secure, fail to decrypt'));
263265
}
264-
var encryptedContent = dom.byPath(doc, ['EncryptedData', 'CipherData', 'CipherValue']);
266+
var encryptedContent = dom.child(dom.child(encryptedData, 'CipherData'), 'CipherValue');
265267

266268
var encrypted = Buffer.from(encryptedContent.textContent, 'base64');
267269
switch (encryptionAlgorithm) {
@@ -294,18 +296,27 @@ function decryptKeyInfo(doc, options) {
294296
// element once and read both the EncryptionMethod and the ciphertext from it:
295297
// the two describe the same key, and taking them from one parent is what
296298
// keeps them consistent.
297-
var encryptedKeyElement = dom.byPath(doc, ['KeyInfo', 'EncryptedKey']);
299+
// Resolve the EncryptedData once so the key comes from the same block that
300+
// is decrypted. With no EncryptedData the input is a bare KeyInfo.
301+
var encryptedData = dom.byPath(doc, ['EncryptedData']);
302+
var keyInfo = encryptedData ? dom.child(encryptedData, 'KeyInfo') : dom.byPath(doc, ['KeyInfo']);
303+
var encryptedKeyElement = dom.child(keyInfo, 'EncryptedKey');
298304

299305
if (!encryptedKeyElement) { // try with EncryptedData->KeyInfo->RetrievalMethod
300-
var keyRetrievalMethod = dom.byPath(doc, ['EncryptedData', 'KeyInfo', 'RetrievalMethod']);
306+
var keyRetrievalMethod = dom.child(keyInfo, 'RetrievalMethod');
301307
var keyRetrievalMethodUri = keyRetrievalMethod ? keyRetrievalMethod.getAttribute('URI') : null;
302308
if (keyRetrievalMethodUri) {
303309
// A same-document reference only. An external URL or a bare fragment is
304310
// not something this resolves.
305311
if (keyRetrievalMethodUri.charAt(0) !== '#' || keyRetrievalMethodUri.length < 2) {
306312
throw new Error('RetrievalMethod URI must be a same-document reference');
307313
}
308-
encryptedKeyElement = dom.elementById(doc, 'EncryptedKey', keyRetrievalMethodUri.substring(1));
314+
// The EncryptedKey may be a sibling of EncryptedData (e.g. Okta), so
315+
// search the enclosing element (EncryptedAssertion) rather than only
316+
// the EncryptedData subtree.
317+
var parent = encryptedData && encryptedData.parentNode;
318+
var scope = parent && parent.nodeType === 1 ? parent : doc;
319+
encryptedKeyElement = dom.elementById(scope, 'EncryptedKey', keyRetrievalMethodUri.substring(1));
309320
}
310321
}
311322

‎test/test-okta-sibling-key.xml‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<saml2p:Response Destination="https://example.com/login/callback" ID="id0000000000000000000000000001" Version="2.0" xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol">
3+
<saml2:EncryptedAssertion xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">
4+
<xenc:EncryptedData Id="_00000000000000000000000000000001" Type="http://www.w3.org/2001/04/xmlenc#Element" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
5+
<xenc:EncryptionMethod Algorithm="http://www.w3.org/2009/xmlenc11#aes128-gcm" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#"/>
6+
<ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
7+
<ds:RetrievalMethod Type="http://www.w3.org/2001/04/xmlenc#EncryptedKey" URI="#_00000000000000000000000000000002"/>
8+
</ds:KeyInfo>
9+
<xenc:CipherData xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
10+
<xenc:CipherValue>vsSeiTCI4zf0LRuFBxUmIhyhbPO6kVN152a3rMPGeNE4I5WrYzbd0JRUTAl6fXKF</xenc:CipherValue>
11+
</xenc:CipherData>
12+
</xenc:EncryptedData>
13+
<xenc:EncryptedKey Id="_00000000000000000000000000000002" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
14+
<xenc:EncryptionMethod Algorithm="http://www.w3.org/2001/04/xmlenc#rsa-oaep-mgf1p" xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
15+
<ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1" xmlns:ds="http://www.w3.org/2000/09/xmldsig#"/>
16+
</xenc:EncryptionMethod>
17+
<xenc:CipherData xmlns:xenc="http://www.w3.org/2001/04/xmlenc#">
18+
<xenc:CipherValue>JhwBazYQHN81PuOKRbHt4lbCttx6F0BZvjeCGrU7tziGi5ncMwd0ld2kcvYaOwncgVWv3nPCVJGGtoJUILg8z71mZJXuoz3MRufQ0G1TACKOAKbWhvI5ZIDEOAzuZzr385zKrNV7Zs7yTGAV/RezogddIUD6e/UokXQgvOcByeOh+2xAP8QLWqrBgrFwSIr+HKwEp3T921y7EHjS5SzRXzjubwXw8PTZlwS/nj8VJ9IBL3yqjEEaX0fsPoCgbmNnJddxW7g9SVaI/1YmAkHGMlLx2ruY++UQDL/uz9Y0Dm91IMF0ZRLA0LHSjHDJvdu2FckWkwM98JpjYQnfopSS8w==</xenc:CipherValue>
19+
</xenc:CipherData>
20+
</xenc:EncryptedKey>
21+
</saml2:EncryptedAssertion>
22+
</saml2p:Response>

‎test/xmlenc.digest.js‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -603,3 +603,91 @@ describe('OAEPparams', function () {
603603
});
604604
});
605605
});
606+
607+
describe('RetrievalMethod to a sibling EncryptedKey (Okta layout)', function () {
608+
var xmldom = require('@xmldom/xmldom');
609+
var options = {
610+
rsa_pub: fs.readFileSync(__dirname + '/test-auth0_rsa.pub'),
611+
pem: fs.readFileSync(__dirname + '/test-auth0.pem'),
612+
encryptionAlgorithm: 'http://www.w3.org/2009/xmlenc11#aes256-gcm',
613+
keyEncryptionAlgorithm: RSA_OAEP,
614+
keyEncryptionDigest: 'sha256'
615+
};
616+
var decryptOptions = { key: fs.readFileSync(__dirname + '/test-auth0.key') };
617+
618+
// Wraps the result in an EncryptedAssertion, with the EncryptedKey placed
619+
// as a sibling of EncryptedData and referenced by RetrievalMethod.
620+
function siblingLayout(result, wrap) {
621+
var m = /<e:EncryptedKey[\s\S]*<\/e:EncryptedKey>/.exec(result);
622+
assert(m, 'expected an EncryptedKey element');
623+
var encryptedKey = m[0].replace('<e:EncryptedKey', '<e:EncryptedKey Id="ek1"');
624+
var encryptedData = result.replace(m[0], '<RetrievalMethod URI="#ek1" />');
625+
return wrap(encryptedData, encryptedKey);
626+
}
627+
628+
function encryptedDataOf(xml) {
629+
var doc = new xmldom.DOMParser().parseFromString(xml);
630+
return doc.getElementsByTagNameNS('http://www.w3.org/2001/04/xmlenc#', 'EncryptedData')[0];
631+
}
632+
633+
it('resolves the key from the EncryptedData parent', function (done) {
634+
xmlenc.encrypt('sibling content', options, function (err, result) {
635+
if (err) return done(err);
636+
var xml = siblingLayout(result, function (data, key) {
637+
return '<saml2:EncryptedAssertion xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:e="http://www.w3.org/2001/04/xmlenc#">' +
638+
data + key + '</saml2:EncryptedAssertion>';
639+
});
640+
var element = encryptedDataOf(xml);
641+
assert.equal(element.parentNode.localName, 'EncryptedAssertion');
642+
xmlenc.decrypt(element, decryptOptions, function (err2, decrypted) {
643+
if (err2) return done(err2);
644+
assert.equal(decrypted, 'sibling content');
645+
done();
646+
});
647+
});
648+
});
649+
650+
it('finds the key in an Okta-shaped response (fails at the RSA unwrap, not the lookup)', function () {
651+
// Okta layout with all identifiers and ciphertext replaced by dummies, so the
652+
// key cannot unwrap; failing at the RSA step proves the key was located.
653+
var xml = fs.readFileSync(__dirname + '/test-okta-sibling-key.xml', 'utf8');
654+
var element = encryptedDataOf(xml);
655+
assert.throws(function () {
656+
xmlenc.decryptKeyInfo(element, decryptOptions);
657+
}, function (err) {
658+
return !/cant find encryption algorithm/.test(err.message);
659+
});
660+
});
661+
662+
it('uses the key of the EncryptedData being decrypted when another block has an inline key', function (done) {
663+
xmlenc.encrypt('first block', options, function (err, first) {
664+
if (err) return done(err);
665+
xmlenc.encrypt('second block', options, function (err2, second) {
666+
if (err2) return done(err2);
667+
var xml = siblingLayout(first, function (data, key) {
668+
return '<Root xmlns:e="http://www.w3.org/2001/04/xmlenc#">' + data + key + second + '</Root>';
669+
});
670+
var doc = new xmldom.DOMParser().parseFromString(xml);
671+
xmlenc.decrypt(doc, decryptOptions, function (err3, decrypted) {
672+
if (err3) return done(err3);
673+
assert.equal(decrypted, 'first block');
674+
done();
675+
});
676+
});
677+
});
678+
});
679+
680+
it('does not resolve an EncryptedKey outside the EncryptedData parent', function (done) {
681+
xmlenc.encrypt('sibling content', options, function (err, result) {
682+
if (err) return done(err);
683+
var xml = siblingLayout(result, function (data, key) {
684+
return '<Root xmlns:e="http://www.w3.org/2001/04/xmlenc#"><A>' + data + '</A><B>' + key + '</B></Root>';
685+
});
686+
xmlenc.decrypt(encryptedDataOf(xml), decryptOptions, function (err2) {
687+
assert(err2, 'expected failure');
688+
assert.match(err2.message, /cant find encryption algorithm/);
689+
done();
690+
});
691+
});
692+
});
693+
});

‎test/xmlenc.integration.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,25 @@ describe('integration', function() {
3030
}
3131
);
3232
});
33+
34+
// passport-wsfed-saml2 passes only the EncryptedData element, while Okta
35+
// places the referenced EncryptedKey beside it under EncryptedAssertion.
36+
it('should decrypt Okta assertion when given only the EncryptedData element', function (done) {
37+
var doc = new xmldom.DOMParser().parseFromString(
38+
fs.readFileSync(__dirname + '/test-okta-enc-response.xml').toString()
39+
);
40+
var encryptedData = doc.getElementsByTagNameNS('http://www.w3.org/2001/04/xmlenc#', 'EncryptedData')[0]
41+
|| doc.getElementsByTagName('EncryptedData')[0];
42+
assert.equal(encryptedData.parentNode.localName, 'EncryptedAssertion');
43+
44+
xmlenc.decrypt(
45+
encryptedData,
46+
{key: fs.readFileSync(__dirname + '/test-okta.pem'), disallowDecryptionWithInsecureAlgorithm: false},
47+
(err, res) => {
48+
assert.ifError(err);
49+
assert(/Assertion/.test(res));
50+
done();
51+
}
52+
);
53+
});
3354
});

0 commit comments

Comments
 (0)