Missing cert configuration option disables signature validation silenty/without huge warning messages #523
Comments
|
Thanks for the detailed report, @srd90. |
|
This comment is reply @markstos 's question at #536 (comment)
At the moment https://github.com/node-saml/passport-saml/network/dependents shows > 1200 public dependents and npmjs shows > 137 public dependents and weekly downloads >110k With such a large usage base and even larger installation base (and quite many of those applications might be something that provides some "authentication setup screen" for those applications' end users) even smallest chance to set things up incorrectly (meaning configuring As communicated by @pitbulk and @alexstuart in #180 and as shown in this issue missing
source: https://wiki.shibboleth.net/confluence/display/IDP4/SAML2SSOConfiguration So IMHO btw. I tried if I could find with search engine some codebase with insecure After browsing Backstage's codebase I found this (link points to their current head of master branch): cert: config.getOptionalString('cert'),and lines 140-144 show that they remove // passport-saml will return an error if the `cert` key is set, and the value is empty.
// Since we read from config (such as environment variables) an empty string should be equal to being unset.
if (!opts.cert) {
delete opts.cert;
}
return new SamlAuthProvider(opts);If backstage with such an active developer base ends up using Disclaimer: I just assumed that Backstage developers introduced a bug. I'm tagging @lowjoel and @freben (because they have contributed / managed pull requests related to Backstage's |
|
@srd90 Thanks for more context. I'm in favor of a "safe by default" approach and am comfortable with making the Are you employed as a white hat by chance? |
|
I have no objections, but the code is currently in the process of being reworked such that even if we release now, we should probably do a semver major. I'm working on a PR to rework things to require |
@markstos No. Checking |
|
Are we saying that we are requiring |
|
I read the conversation above as applying only to `cert`.
…On Mon, Feb 22, 2021 at 4:23 PM Chris Barth ***@***.***> wrote:
Are we saying that we are requiring cert only or also requiring privateKey
as well? I want to make sure I enforce the correct things in the work I'm
doing.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#523 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAGJZOWMLZOJWDBD63WUF3TALDMJANCNFSM4WSIOARA>
.
--
*Mark Stosberg* (he/him)
Director of Systems & Security
[email protected] | 765.277.1916
https://www.rideamigos.com <https://rideamigos.com/>
Changing the way the world commutes.
<https://www.linkedin.com/company/rideamigos>
<https://www.twitter.com/rideamigos> <https://www.facebook.com/rideamigos>
<https://www.instagram.com/rideamigos>
<https://rideamigos.com/newsletter-sign-up/>
|
There is/was issue #180 which addressed this particular case.
There is no need to repeat reasons why
passport-samlMUST have safe defaults for signature validation configuration ( read @pitbulk 's and @alexstuart 's comments from #180 ). In fact one should not be able disable it at any circumstances.It seems that there was some sort of consensus at the end of #180 that
but content of commit ( f6b1c88 ) which closed issue #180 allows completely missing or misspelled
certconfiguration option which is effectively same thing ascertwithfalsyvalue (signature validation is completely disabled allowing anyone to alter login response any way he/she chooses to).Following examples are executed against
passport-samlversion https://github.com/node-saml/passport-saml/tree/932da9d09a018fed4cb830e67090bb994f8539c1 (v2.0.4) using this particular test case (with various modifications) to demonstrate effects of missingcertconfiguration option:passport-saml/test/tests.js
Lines 1623 to 1643 in 932da9d
Reference run (version 932da9d without any modifications):
Reference run 2 ( 932da9d with altered response):
Value of response's nameID is altered to
[email protected]Test failed as expected due invalid signature.
Case:
certconfiguration option is not provided at all.This could happen
cert)Test case is modified so that SAML is configured without
certoption and NameID is altered to[email protected].Test should fail due invalid signature or passport-saml should fail fast due missing
certconfiguration option value.It SHOULD NOT pass so that
passport-saml's client application receives altered nameid value......but instead of failing it passed altered nameid to
passport-saml's cient application (even though signature didn't match anymore).Case: misspelled
certconfiguration option name.Basically same as missing
certconfiguration value.This should have failed due same reasons as "missing
certconfiguration option case".It might be possible to spot incorrectly configured passport-saml stacks by sending SAML login responses with altered content and observing how servers respond to those login responses if services return enought error information to spot such a configuration error. Or in worst case one could manage to get authenticated session with first try.
passport-samlinstance without login response signature validation is as good / bad as no authentication at all.So instead of this:
passport-saml/src/passport-saml/saml.ts
Lines 125 to 127 in 932da9d
should it be e.g.:
or
btw.
passport-saml's documentation https://github.com/node-saml/passport-saml/blob/932da9d09a018fed4cb830e67090bb994f8539c1/README.md#security-and-signaturesalong with how
certis not a must have configuration option could give someone an idea thatcertis not anything special.The text was updated successfully, but these errors were encountered: