Problem: SHA-1 is considered insecure and should not be used by default, see https://shattered.io/

We should enable strict checking of responses and use SHA-256 instead.

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
StatusFileSize
new982 bytes

Patch.

smfsh’s picture

Status: Needs review » Needs work

I think this is great, and its true that this is recommended. The D8 module implements this on a per-site bases though as a configuration option and I think we should do the same. Checkbox for strict checking and drop down for algorithm (like the d8 setup.)

I can get this implemented along with a couple other changes I was planning to make to the form unless you get around to it first.

nironan’s picture

Status: Needs work » Needs review
StatusFileSize
new3.35 KB

Here's an attempt to make the settings configurable. I made SHA-256 the default... is it a good idea? Or should we leave the (insecure) SHA-1 as default for backward compatibility?

roderik’s picture

Status: Needs review » Postponed (maintainer needs more info)

(I'm taking over D7 maintenance.) Thank you for raising this; contributions like these still require me to look more closely at the library code which is a good thing.

I have a question though. But given that this has been sitting in the queue so long, I'll already commit the non-question part.

On the original issue / patch:

  • Question: why the addition of the base_url setting? Is there a situation where it makes a difference? I don't think it does anything wrong per se, but also... while testing some things, I could never see it actually getting used. So I won't commit it yet, also because 1) base_url changes always feel creepy to me because of their potential side effects in edge cases; 2) the D8 version doesn't have it either.
  • For those interested: the algorithm used was (and will still be by default) whatever the php-saml library uses by default. This was SHA1 for v2 of the library (installed by 7.x-1.0-beta1) and SHA256 for v3 (installed by 7.x-1.0-dev since April 2019 / #3044116: Update SAML Auth library to version 3 for PHP 7.2 support)

On patch #4: thanks, I agree that it's good to have both be configurable. (Also, I now see that the D8 version's default choice is still SHA1 because that was the default in 2017. Sigh.) But

  • I added 'library default' to the list of choices. I'll make that the default choice; it means (just like now) that whenever someone upgrades php-saml to a version whose default is SHA-512, in 5?? years... they'll be updated automatically.
  • (which at the same skirts around the issue that the default variable value should have been "'http://www.w3.org/2001/04/xmlenc#sha256'", not "sha256")
  • Added a "(bool)" to the strict option. (Not because I know passing NULL is bad - but I couldn't with 100% certainty make out that it never is.)
  • The "digest algorithm" isn't used anywhere. It is only used when signing the metadata - which is a setting that is off by default and that we haven't implemented configuration for, so far. (We could, but I'm leaving it out of this issue.)

Setting Needs Info for the first bullet point (and sure I'll commit a patch with 'security']['signMetadata' if someone wants / provides it), committing the rest.

roderik’s picture

StatusFileSize
new2.66 KB
new2.92 KB

  • roderik committed 9304598 on 7.x-1.x authored by klausi
    Issue #3043713 by klausi, nironan, roderik: Do not use insecure SHA-1...

  • roderik committed ac845a8 on 8.x-3.x
    Issue #3043713 by roderik, nironan, klausi: Make 'library default' the...

  • roderik committed 3668c30 on 7.x-1.x
    Issue #3043713 by klausi: set baseurl in Saml2\Settings.
roderik’s picture

Status: Postponed (maintainer needs more info) » Fixed
Related issues: +#2912717: Use the `Forwarded-For-*` headers for proxy

Responding to myself:

why the addition of the base_url setting? Is there a situation where it makes a difference? I don't think it does anything wrong per se, but also... while testing some things, I could never see it actually getting used.

1) This is not true, I can see it getting used (if I just take care of activating xdebug properly on those post requests...)

2) (I'm assuming that) I just realized what this the advantage of this... it sets the host/port/etc correctly if the SP is behind a reverse proxy.

In the D8 version I committed #2912717: Use the `Forwarded-For-*` headers for proxy for this, which calls \OneLogin_Saml2_Utils::setProxyVars(TRUE) (public static function, meh) and adds a note to the README to configure settings. That may not be needed after we set 'baseurl'.

So I'll commit this and put a TODO in the D8 version to test/change that sometime.

  • roderik committed 4b9b6ba on 8.x-3.x
    Issue #3043713: Fix default (empty) value for signature algorithm,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.