Fix malformed X-WSSE header vulnerability

[magabriel]

This PR fixes two vulnerabilities related to not set or malformed X-WSSE header.

Both problems happen because the current code assumes an "allowed by default" policy, while "denied by default" is a safer assumption.

1.- No X-WSSE header in request

if no X-WSSE header is set, authentication must be denied; otherwise it would be trivial to bypass the security just by sending a request without the X-WSSE header.

2.- Malformed X-WSSE header

If the X-WSSE header is present but malformed the authentication must also be denied, or again it would be trivial to bypass.

I added one unit test for each one of the fixes.

BTW, thanks for this marvelous bundle!!!

[djoos]

Thanks @magabriel, I'll try to have a look at the PR and get back to you ASAP...

Have a great evening!
David

[djoos]

Hi @magabriel,

thanks again for your PR.

Would you be able to explain a the details of this PR a bit more?

1. No X-WSSE header in the request
   When there's no X-WSSE header in the request, the Escape\WSSEAuthenticationBundle\Security\Http\Firewall\Listener won't kick in (handle-method) and will basically let Symfony take care of the rest via the access_control in security.yml). Eg. what if you have a firewall with WSSE Auth, but do allow particular paths via the access_control?

2. Malformed X-WSSE header
   When passing a malformed X-WSSE header, the WSSE Authentication does, correctly, not allow access! Do let me know if you have found this to work otherwise though...

Thanks in advance for your feedback!

Kind regards,
David

[magabriel]

Hi @djoos.

1. That's the problem: if no X-WSSE is sent in the request, the whole WSSE authentication is bypassed. The use case you propose is not a realistic one because you cannot assume that the Symfony security will be there to take care. Think on a pure RESTful API protected only via WSSE (this is my use case btw: AngularJs frontend and Symfony backend, where no other authentication mechanism can be used because all the interaction is done via the RESTful API). But also, from a pure security design point of view, if you hit an API that is supposed to be protected with WSSE authentication but don't provide an X-WSSE header the authentication must be denied, not granted.

2. In Security/Http/Firewall/Listener.php line 49 you have $wsseHeaderInfo = $this->parseHeader(); but this method returns false if the X-WSSE header cannot be parsed (i.e. it is malformed) because of the return false;in the try...catch that encloses the parsing of all elements. So if the header is malformed `$wsseHeaderInfo' will be false, so the subsequent validation will never be executed:

$wsseHeaderInfo = $this->parseHeader();
if($wsseHeaderInfo !== false)
{
    // the validation is here but it is not executed
}

So again, an easy way to bypass the validation is just sending a malformed X-WSSE header to avoid the authentication. And again, you cannot rely on another layer of authentication being in place to deny access because in a pure RESTful API there will be none.

I provided two unit test for the above changes. Did you have the opportunity to review them?

Regards.

[djoos]

Hi @magabriel,

thanks for your feedback - I had a look at the tests you provided, thanks for that!

I do appreciate your remarks, but the statement that you'd have a Symfony2 application with a WSSE secured firewall where you can simply bypass the WSSE authentication by either omitting the X-WSSE or sending a malformed X-WSSE is not (entirely) correct... :-)

Authentication vs authorization
You're right: authentication will not succeed in those cases as the WSSE authentication won't have kicked in, but whether that then "exposes" your API or not is determined by the fact whether authorization is needed or not, but is another step in the whole security process, outside of any authentication bundle's scope. (access_control)

(see: http://symfony.com/doc/current/book/security.html)

When you have a look at the (standard) Symfony2 authentication providers you'll also see they don't block when not being triggered, but don't start executing their authentication either - just like the design of this particular bundle.

Hope this helps - thanks in advance for your feedback!
David

[magabriel]

Hi @djoos,

You are right in your comments, of course. I'm aware of the points you raised.

But in the following page: http://symfony.com/doc/current/cookbook/security/custom_authentication_provider.html where they show how to implement a WSSE authentication scheme (and which I presume your code is based on), under "The Listener" heading there is the following remark:

Returning prematurely from the listener is relevant only if you want to chain authentication providers (for example to allow anonymous users). If you want to forbid access to anonymous users and have a nice 403 error, you should set the status code of the response before returning.

And this is precisely my point: in this case (a RESTful API secured with WSSE) you don't need nor want to chain providers because you are relying solely on the WSSE authentication. My two proposed changes remove the "premature return" from the listener in the case of absent or malformed X-WSSE header and instead set the authentication return code before returning.

Regards.

[djoos]

Hi @magabriel,

thanks for your reply and for clarifying your point further!

I must say that "vulnerability" in the title of this issue started me off on the backfoot :-)

I do like your implementation of a "default deny authorization" (when WSSE-header is missing or malformed), but I would prefer to make this configurable for the bundle.

As mentioned before, we actually have a few WSSE-secured APIs where particular calls are allowed for anonymous users. (not chaining providers, but just allowing anonymous user-calls) So, I actually don't think that is something that crazy to allow via the default authentication/authorization steps.

The documentation should be updated so the setting is clear to bundle users so they can turn off the "default deny authorization", depending on their specific implementation.

What do you think about that compromise? ;-)

Thanks in advance for your feedback!
David

[magabriel]

OK, I admit that "vulnerability" is strong word to be in PR title and may be I should have given it a second thought. Because I see now that it is a vulnerability just for my use case (not chaining providers) but a feature for the rest that, in fact, need chaining providers. My bad ;(

And of course I agree of making that feature configurable. I will update the PR with that soon.

Thanks.

[djoos]

Perfect, I'm glad we could get to a compromise :-)

Thanks in advance!
David

[djoos]

Hi @magabriel,

how are you getting on with the PR? Let me know if you need a hand!

Kind regards,
David

[magabriel]

Hi, David.

Unfortunately, life got in the middle and I've been unable to find the time
to work on the PR. It shouldn't be difficult to do it and I am definitely
determined to do it ASAP.

I'll let you know.

Regards.

-- Miguel Ángel Gabriel --

2014-06-05 18:53 GMT+02:00 David Joos [email protected]:

Hi @magabriel https://github.com/magabriel,

how are you getting on with the PR? Let me know if you need a hand!

Kind regards,
David

—
Reply to this email directly or view it on GitHub
#42 (comment)
.

[djoos]

No worries - thanks in advance!
David

[magabriel]

David,

Life is complicated and keep steering me away. So I'm afraid I no longer
have the time or the motivation to work on this PR.

Sorry about that, It was never my intention starting something I couldn't
finish, but life is life. I think this PR should be closed by the moment.

Regards.

-- Miguel Ángel Gabriel --

2014-06-05 19:58 GMT+02:00 David Joos [email protected]:

No worries - thanks in advance!
David

—
Reply to this email directly or view it on GitHub
#42 (comment)
.

[djoos]

Hi @magabriel,

thanks for the update! Don't worry - I'll try and finish of the work we discussed in this thread so at least the work you've already pushed in our direction will end up in the bundle...

Thanks for your contribution!

All the best,
David

[bkosborne]

I was curious about this and did a little test.

I have two firewalls, one for my API and one for normal app users:

firewalls:
    dev:
        pattern:  ^/(_(profiler|wdt)|css|images|js)/
        security: false

    api:
        pattern: ^/api/.*
        provider: api_users
        stateless: true
        wsse:
            encoder:
                algorithm: sha256
            lifetime: 120

    main_site:
        provider: normal_users
        pattern: ^/
        anonymous: ~
        cas:
            login_path: dashboard
            check_path: /login_check
        logout:
            path: /logout
            invalidate_session: true
        switch_user: { role: ROLE_ADMIN }

With NO access control on my /api paths, and a user trying to hit an API path without a WSSE header, they'll get a 401. This seems expected to me since I did not tell my firewall to allow anonymous users.

If I added anonymous: ~ to my API firewall config, then they would be allowed in. To protect my routes, I would then add access control.

[djoos]

Hi @bkosborne,

that is exactly how the bundle was developed to do. However, from the conversation with @magabriel, it seems that it might be needed to have a more strict behaviour available (configurable and not used by default); in which case a request without the wsse header 401s for a firewall making use of wsse - forcing the header to be present.

Please do let me know your thoughts!

[bkosborne]

I still don't see how adding config option to explicitly deny access is different from disabling anonymous access for the firewall.