Skip to content

Commit

Permalink
[IMP] http: reinstate session rotation after login
Browse files Browse the repository at this point in the history 
Session rotation was introduced a long time ago, but deactivated at
login due to obscure side-effects related to #6949 (aka the "BBQ" PR).
This commit reinstates the rotation, which is better from a security
standpoint.

In order to also prevent session ID reuse, we force the renewal of
deleted sessions, at the SessionStore level (via `renew_missing`).
  • Loading branch information
@odony
odony committed Oct 1, 2018
1 parent c476008 commit 9bae56a
Showing 1 changed file with 5 additions and 1 deletion.
6 changes: 5 additions & 1 deletion odoo/http.py
View file
Expand Up @@ -1038,6 +1038,7 @@ def authenticate(self, db, login=None, password=None, uid=None):
uid = odoo.registry(db)['res.users'].authenticate(db, login, password, env)
else:
security.check(db, uid, password)
self.rotate = True
self.db = db
self.uid = uid
self.login = login
Expand Down Expand Up @@ -1299,7 +1300,8 @@ def session_store(self):
# Setup http sessions
path = odoo.tools.config.session_dir
_logger.debug('HTTP sessions stored in: %s', path)
return werkzeug.contrib.sessions.FilesystemSessionStore(path, session_class=OpenERPSession)
return werkzeug.contrib.sessions.FilesystemSessionStore(
path, session_class=OpenERPSession, renew_missing=True)

@lazy_property
def nodb_routing_map(self):
Expand Down Expand Up @@ -1415,6 +1417,8 @@ def get_response(self, httprequest, result, explicit_session):
if httprequest.session.rotate:
self.session_store.delete(httprequest.session)
httprequest.session.sid = self.session_store.generate_key()
if httprequest.session.uid:
httprequest.session.session_token = security.compute_session_token(httprequest.session, request.env)
httprequest.session.modified = True
self.session_store.save(httprequest.session)
# We must not set the cookie if the session id was specified using a http header or a GET parameter.
Expand Down

9 comments on commit 9bae56a

@lem8r
Copy link
Contributor

@lem8r lem8r commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odony Hi!
After this commit /web/session/authenticate returns outdated session_id.

That happens because response object is prepared before session is rotated. However cookies are set with rotated session, so latter requests are working fine.

But there are problems for apps that are doing CORS requests without cookies. Basically now it is not possible to set explicit session (args or headers) with session received from /web/session/authenticate call.

Is it intended behavior or we can expect some workaround?

Thank you!

@odony
Copy link
Contributor Author

@odony odony commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lem8r It's intended, and we'll be making more changes towards preventing business logic from even knowing the session ID - in order to reduce the chance of XSS exploits.
The first step was to mark the cookie as HTTPOnly, and the next one is to remove the session_id from the session_info(), and finally, from the memory of the web client. This work was completed for 13.0 here: 3166442#diff-9b4ffb24f8f826f3086cacdc7174eafbL26

In the long term the proper solution for authenticated CORS requests is to enable with_credentials on the XHR side, and mirror that with Access-Control-Allow-Credentials on the server-side, for preflights [1]. This way CORS requests will use cookies transparently without having to explicitly manage the session.

Does that make sense to you?

[1] More info on the MDN page: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials

@lem8r
Copy link
Contributor

@lem8r lem8r commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odony Thank you! Now it is clear where things are going.
For now it seems that we need to monkey-patch http.py to send extra headers for the preflight requests.

@odony
Copy link
Contributor Author

@odony odony commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now it seems that we need to monkey-patch http.py to send extra headers for the preflight requests.

Or add the header in the proxy that does the SSL termination in front of Odoo, in order to avoid a monkey-patch ;-)

@lem8r
Copy link
Contributor

@lem8r lem8r commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good idea!

@lem8r
Copy link
Contributor

@lem8r lem8r commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some experiments. Adding headers is not enough.
When browser is doing OPTIONS request to a json controller with auth='user' it does not sends the credentials yet, so controller responds with redirect to /web/login.

Solution is to respond with headers for OPTIONS request without calling ['ir.http'].dispatch() which checks session before performing response.
In that case the following request is sent with credentials and everything works great.

@odony
Copy link
Contributor Author

@odony odony commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, if you want to make a PR for this we'll be glad to look at it!

e.g. some variation of https://github.com/odoo/odoo/blob/master/odoo/http.py#L774-L781 for JSON routes

@lem8r
Copy link
Contributor

@lem8r lem8r commented on 9bae56a Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I can do a PR but it will include modified Access-Control-Allow-Origin otherwise calls to /web/session/authenticate, /web/dataset/call_kw, etc.. will be blocked.
The reason is Access-Control-Allow-Credentials requires explicit origin to be set. Wildcard '*' is not allowed.

It is fine to do so in our case, because we do CORS JSON-RPC requests only for local development. (Production scripts are server from same domain). I'm not sure if it is fine to have it in upstream code.

Example

class Response(werkzeug.wrappers.Response):
    def set_default(self, template=None, qcontext=None, uid=None):
       # ...
        if request.endpoint.routing['type'] == 'json':
            self.headers.set('Access-Control-Allow-Credentials', 'true')
            self.headers.set('Access-Control-Allow-Origin', request.httprequest.headers['ORIGIN'])

@lem8r
Copy link
Contributor

@lem8r lem8r commented on 9bae56a Oct 3, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a PR #37853 to show what needs to be changed to make CORS requests.

Please sign in to comment.