Add support for allowedHostnames in StrictHttpFirewall

[eddumelendez]

Introduce a new method setAllowedHostnames which perform the validation
against untrusted hostnames.

Fixes gh-4310

[eddumelendez]

are changes needed in DummyRequest?

[roricolivares]

Any update on this change? Our latest release is being flagged due to this issue now showing up on SourceClear. https://www.sourceclear.com/vulnerability-database/security/incorrect-headers-handling/java/sid-20558.

[jzheaux]

Thanks for the PR, @eddumelendez! I've left one feedback item inline.

Also, in answer to your question, yes, DummyRequest will require an update, otherwise DefaultFilterChainValidator will throw an exception. Since there is no way to give DummyRequest the server name, though, it would need to return null.

This would necessitate StrictHttpFirewall doing a null check before calling the predicate, otherwise an application would have to quite surprisingly check for a null server name in their predicate. Since getServerName isn't nullable in the ServletRequest contract, this isn't ideal, but I don't see another way. Perhaps @rwinch has some ideas.

[eddumelendez]

thanks for the feedback @jzheaux. Changes has been submitted

[jzheaux]

Thanks for the updates, @eddumelendez! Please see my feedback inline.

[eddumelendez]

fixed :)

[jzheaux]

Thanks again, @eddumelendez! In preparation for merging, would you go ahead and squash your commits?

[eddumelendez]

@jzheaux done :)

[jzheaux]

Nice work, @eddumelendez, this is now merged into master.

[avodonosov]

@jzheaux , @eddumelendez , thanks for fixing that. I see the 5.x series release is created.

Can we hope it will also be released in 4.x soon?

How about 3.x?

[jzheaux]

@avodonosov There aren't plans to backport this since it introduces a public API, StrictHttpFirewall#setAllowableHostnames.

Note, though, that adding the functionality yourself is quite simple as the HttpFirewall can be customized:

void configure(WebSecurity web) {
    web
        .httpFirewall(new MyCustomFirewall());
}

[avodonosov]

@jzheaux , why introducing a public API prevents backporting? (If it was breaking public API then I understood).

Thanks for the reply though.

Yes, it's possible to implement the same whitelist solution ourselves.

[jzheaux]

Yes, it's possible to implement the same whitelist solution ourselves.

I'm glad to hear that. Let me know if you run into any trouble.

why introducing a public API prevents backporting?

Spring Security follows semantic versioning. One of the key points is number 7 (emphasis mine):

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

We've made rare exceptions to this in the past, but this doesn't appear to be one of those.

What it means is that the public API doesn't get added to except in minor version releases, e.g. 5.2, 5.3, etc.

If there were a known vulnerability related to the Host header, it might be fixed in a different way from this general solution, so it's not really sensible to backport the whitelisting feature on those grounds.

[avodonosov]

@jzheaux, I'd like to share 2 things about semantic versioning:

1. It doesn't prohibit backporting to version 4. (I'm not sure, but from
   your comment seems you imply semantic versioning prohibits it)
   The current release is 4.2.13, the fixed release will be 4.3.0 - all according to the quote you provide.

2. Semantic versioning doesn't work for Java when we want an old version
   of a library to be used simultaneously with the new version in one application.
   Simply because Java doesn't allow several versions of classes with an equal
   fully-qualified name. (In the same class loader. Using different classloaders,
   as OSGI does, is an overkill unpractical in most real situations, of course. Honestly,
   I haven't used java modules, but the impression they don't solve the problem
   in a convenient way too, unfortunately).

   How often we need several version simultaneously? Almost always.

   First, dependency tree of any moderate size application has some
   libraries present in multiple tree nodes, with different versions.

   The second (more relevant for spring-security): migration easiness.
   We may want our old REST API endpoints to remain protected by spring-security 4,
   because it's already tested stable code. But for new endpoints, we'd like to use the modern,
   latest version 5.

   If this is not allowed, then we have a much more difficult "all or nothing" situation:
   unless you found resources to migrate and test your existing code, you must continue
   using version 4 for old endpoints, thus bogging down deeper and deeper into the old version,
   which has limited or sometimes no maintenance (in some large system I know some
   components still use spring-security 3).

In order to allow several versions of your library in an application, give them different artifact name (spring-security3, spring-security4, spring-security5) and different package names (org.springframework.security3, org.springframework.security4, org.springframework.security5).

commons-collections, commons-lang use this approach

In short - the principle of immutability applied to versioning, dependency management and backward compatibility. Here immutability of values (APIs) assigned to names (class / artifact names). If version 5 is incompatible with version 4, they should not share fully-qualified class names and artifact names.

[jzheaux]

You are correct that this change could be made in a hypothetical 4.3.x, though no such release is planned.

I'm not certain what you are driving at with the rest of your comments, but if you'd like to pursue them further, I'd recommend you create a separate ticket as it feels like we are getting off-topic here.

The decision to follow semantic versioning is a Spring-wide one, though, so if you are trying to make a case against it, you'd need to make it first with Spring Framework.

[avodonosov]

Well, I have this point about semver for years, so the mention just triggered me to present it.

There is a valid thinking behind it, but it should be applied with understanding. Simultaneous loading of several versions is crucial. Javascript allows this for free - js packages (in all major package management solutions) do not have a global namespace to interfere (as Java's fully-qualified class names). Js semver doesn't translate to Java so directly and mechanically as people think. For Java the apache common's approach is the best.

It's not against semver. It just takes it further, to the point it works. The approach I advocate fully complies to the semver (a subclass, so to speak) - if application refers a dependency, the application receives what it expects in any future evolution made according to the rules.

Sorry I'm continuing the offtopic. Also, I'm not ready to push the whole Spring project - I'm afraid it will require more energy than I have and I'm not sure of success with whatever energy. But I decided to mentions it as you referred semver.