Cookie echoing with wrong path in provider.authz_part2

[schlenk]

When the oidc provider is not placed at the '/' path, e.g. putting it under '/oidc' instead, the code setting the SSO cookie in self._complete_authz() does a surprising thing: It echoes all cookies it receives with the current path.

So for example, you have an app mounted at '/' that sets a cookie 'lang=en', path='/'. Now you run a oidc flow against a provider with baseurl '/oidc'.

The code now sees a cookie in environ['HTTP_COOKIE'] from wsgi, extracts it via wsgi_wrapper and puts as kwargs into the authorization endpoint. This does all the usual things, then descends to authz_part2, and passes the cookies into the complete_authz in the baseclass. The code inspects all the cookies, then copies them into headers, so they get set again, but with a different path argument (/oidc).

I do not understand why this cookie echoing is needed at all.

Can anyone shed some light into this, why is this needed in oic.oauth2.provider.AProvider._complete_authz()? And if your at it, why is very similar code commented out in oic.oic.provider around line 789?

Both have been introduced with 59f83ac by @rohe

[schlenk]

Just noticed that this also strips the 'secure' Flag from cookies, so this is a security issue too.

[rohe]

I have a very diffuse memory now about why I did it. It came from the OIDC test tool and if I remember correctly it was the only way I could get it working against a specific OIDC OP.
So there was a reason behind echoing but it should definitely not have included setting a different path argument or stripping the 'secure' flag.

[rohe]

Commenting out the code at oic.oic.provider 789 was part of a refactoring.

[schlenk]

Ok, if it echoes things exactly how it finds them, I'm fine with it. Only the 'wrong path/strip flags' part is not good.

We had some similar issues with the need to echo Cookies with some Javascript inside Microsoft IE, which lost cookies in XHR requests sometimes.

[tpazderka]

@schlenk Continuing discussion from #593:
We should probably fix this as we are now doing more stuff with cookies.

I am not familiar with low-level cookies, do you have a failing test for this beahovior?

[schlenk]

@tpazderka I have some integration tests with requests.Session against a whole OP, but not nicely isolated. Looking at the tests, i found that the whole echoing of cookies is dangerous and should be avoided, as you cannot really make it safe.

A cookie can have various security properties set up in the Set-Cookie header, e.g. secure, httponly, samesite, domain and path. Those are NOT visible in the received cookies on the server side, e.g. the Cookie headers. This is spelled out in https://tools.ietf.org/html/rfc6265#section-5.4, the User Agent looks at the URI and computes the resulting Cookie string, stripping all the security attributes and turning it into a simple key-value list.

As the server cannot safely decide which cookies are safe to echo, we should simply NEVER echo a cookie.
So the test_complete_authz_other_cookie_exists is already good, but line 450-452 should be inverted assert len(header) == 2 should be assert len(header) == 1.

A more complete integration test test goes like this:

1. Have an OP listen on https://oidc.example.org
2. Have another App on https://www.example.org
3. Login to the https://www.example.org app, and get a cookie like this:
   Set-Cookie: SID=31d4d96e407aad42; Path=/; Domain=example.com; HttpOnly; Secure
4. Now login to the https://oidc.example.org OP, and run through complete_authz
   This sets a new cookie, for the (implicit) domain oidc.example.org
   Set-Cookie: SID=31d4d96e407aad42;
5. Now access http://oidc.example.org/.well-known/ and capture the unprotected cookie (mandatory TLS due to Secure is gone) with wireshark...

This can be simulated by setting up a fake cookiejar with requests.Session() and running a login flow against the op with the same cookie jar. Just add some secure/httponly cookies and log the used domains/paths/flags. Like this (not complete):

def extract_cookie(req, name, domain=None, path=None):
    """ Get the cookie object (instead of just the value) from requests
    """
    toReturn = None
    for cookie in iter(req):
        if cookie.name == name:
            if domain is None or cookie.domain == domain:
                if path is None or cookie.path == path:
                    if toReturn is not None:
                        raise requests.cookies.CookieConflictError(
                            'There are multiple cookies with name, %r' % (name))
                    toReturn = cookie
    if toReturn:
        return toReturn

class EchoTest(unittest.TestCase):

    def setUp(self):
        self.req = requests.Session()
        # add a fake csrf cookie
        self.req.cookies.set('CSRFToken', 'nososecret', path='/', secure=True)

    def test_echo(self):
        # Do some login via oidc
        self.req.post("oidc.example.com/authorize")
        cookie = extract_cookie(self.req.cookies, 'CSRFToken', path='/')
        assert cookie
        assert not cookie.has_nonstandard_attr('httponly')
        assert cookie.secure

[tpazderka]

OK, I do agree that handling of the cookies on our side should be better. Will try to look into it.