Skip to content

Update README.md to reflect getKeyInfoContent changes #470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update README.md to reflect getKeyInfoContent changes #470

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,8 @@ The `SignedXml` constructor provides an abstraction for sign and verify xml docu
- `inclusiveNamespacesPrefixList` - string - default `null` - a list of namespace prefixes to include during canonicalization
- `implicitTransforms` - string[] - default `[]` - a list of implicit transforms to use during verification
- `keyInfoAttributes` - object - default `{}` - a hash of attributes and values `attrName: value` to add to the KeyInfo node
- `getKeyInfoContent` - function - default `noop` - a function that returns the content of the KeyInfo node
- `getCertFromKeyInfo` - function - default `SignedXml.getCertFromKeyInfo` - a function that returns the certificate from the `<KeyInfo />` node
- `getKeyInfoContent` - function - default `SignedXml.getKeyInfoContent` - a function that returns the content of the KeyInfo node
- `getCertFromKeyInfo` - function - default `noop` - a function that returns the certificate from the `<KeyInfo />` node
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that production code still provides possibility to use unsecure implementation (SignedXml.getCertFromKeyInfo):
Quote from version 6.0.0:

xml-crypto/src/signed-xml.ts

Lines 217 to 223 in 0ed7ab2

static getCertFromKeyInfo(keyInfo?: Node | null): string | null {
if (keyInfo != null) {
const cert = xpath.select1(".//*[local-name(.)='X509Certificate']", keyInfo);
if (isDomNode.isNodeLike(cert)) {
return utils.derToPem(cert.textContent ?? "", "CERTIFICATE");
}
}

I.e. aforementioned implementation was not removed from production code meaning that anyone could use it as a value to getCertFromKeyInfo option.

Maybe README.md should state something like

DO NOT at any circumstances configure SigndXml.getCertFromKeyInfo as a value to getCertFromKeyInfo because that SHALL trigger this Critical vulnerability: CVE-2024-32962 aka GHSA-2xp3-57p7-qf4v

or same in markdown

DO NOT at any circumstances configure `SigndXml.getCertFromKeyInfo` as a value to `getCertFromKeyInfo` because
that SHALL  trigger this **Critical** vulnerability: CVE-2024-32962 aka [GHSA-2xp3-57p7-qf4v](https://github.com/node-saml/xml-crypto/security/advisories/GHSA-2xp3-57p7-qf4v)

@cjbarth quick grepping of xml-crypto codebase didn't reveal any usage for SignedXml.getCertFromKeyInfo other than test codes (which could have test code specific implementation of getCertFromKeyInfo) and

xml-crypto/src/signed-xml.ts

Line 57 in 0ed7ab2

getCertFromKeyInfo = SignedXml.getCertFromKeyInfo;

which might be possible to be replaced with SignedXml.noop

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading this correctly, and my greping is correct, nothing in the entire node-saml project depends on this. If it weren't for the fact that this is part of the spec, we could probably remove it entirely. However, since it is part of the spec, I feel like we have to leave this particular foot-gun in place. I'm open to discussion on this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fail to see any mention at the spec that forces anyone to have dead code in their projects / production codebase.

Anyone is able to implement their own foot gun instead of blindly setting aforementioned static foot gun implementation to configuration option. In that case they have full visibility to (they coded/copy pasted it and reviewed that with peer developers) and responsibility of implications of foot gun implementation.

Alternatively someone could just set from node-saml's / xml-crypto's point of view dead code foot gun implementation for whatever reason to that configuration option and forget it and if not even peer reviewers bother to deep dive to internals of recognized/de-facto nodejs xml-crypto library to see and figure out all implications then that unlucky project might be in a world of trouble.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be up for a breaking change that changes this to noop() and removes relavant test. If you need to get your cert from the KeyInfo, that is on you as a consumer. Does that seem reasonable? @ahacker1-securesaml, does that also seem reasonable to you?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with srd90, we should remove the insecure implementation of: static getCertFromKeyInfo.
Using this function would allow the XML signature to be validated with any certificate/public key, even those that you don't trust.
This negates the entire chain of trust.

With regards to a breaking change: There's no way to use the SignedXml.getCertFromKeyInfo and still be secure. If they are using it, then they are insecure. So it's best to remove it, even if it incurs a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm up for that. Let's just move this default implementation to noop and leave it for the consumer. If KeyInfo were signed or this was a trusted internal system (which still has issues, but...), then They can roll their own code, even pulling from a previous release and maintaining it themselves.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using my mobile device so unable to deep dive into code...BUT my concern was that while documentation regarding getCertFromKeyInfo is updated to reflect implementation (default value is now documented to noop) codebase still ships SignedXml.getCertFromKeyInfo static function (which can be used to break chain of trust if configured to signedxml).

If I read your ( @cjbarth ) comments correctly you are now (at the latest comment) talking about getKeyInfoContent configuration option (instead of getCertFromKeyInfo configuration option) and SignedXml.getKeyInfoContent static function (instead of SignedXml.getCertFromKeyInfo static function).

I am unable to deep dive into code so dunno if there is some issues regarding that. My point has been that SignedXml.getCertFromKeyInfo should be removed from codebase (at least from packaged/production codebase).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you @srd90 for your precision. I agree, what we're talking about is SignedXml.getCertFromKeyInfo.


#### API

Expand Down