This is a real issue, but currently we can't implement a CSRF token solution without breaking the UI plugins, as they expect to be able to use the authentication and session created by webadmin, even when running from different machines.

Can we make this bug public?

Hi Juan,

CSRF is an issue indeed, however i'm not sure that fix described in your patch
[1] is a right way to go, as it not just breaks backward compatibility, but
completely disables native authentication method (per request), which is common
and expected in RESTful applications.

also forcing users explicitly mark theirs product as 'insecure' (just to continue
working as they used to) does not seem right to me,

as about the fix itself, existence of Session & JSESSIONID header in request does not verify that the end-user intended to submit the request, 

there are verity of options to deal with this, such as 'Checking The Referer Header', 'Double Submit Cookies ', 'Encrypted Token Pattern', 'Synchronizer Token Pattern', etc.

Thanks.

[1] http://gerrit.ovirt.org/#/c/26578/2//COMMIT_MSG

The proposed fix doesn't disable per request authentication. The check is only performed if the request contains the JSESSIONID cookie, and this cookie won't be contained if the caller is using per request authentication. These are the first few lines of the doFilter method:

   // Nothing to check if there isn't a session yet:
   HttpSession session = request.getSession(false);
   if (session == null) {
     chain.doFilter(request, response);
     return;
   }

The fix does break backwards compatibility, that I agree, but so will do any mechanism that requires new headers/parameters/cookies. In my opinion there is no way around that. And that is why we need a way to allow users, as a last resort, to mark their installation as insecure and thus continue using it. Note that the proposal is to let users do this in a fine grained way, so, for example, if a user has updated most of his RESTAPI callers, but still has a old application that can't be updated, he can just allow request from that particular application:

  function isTrusted(request) {
    // Check if the request is coming from the old application, using
    // the source IP address, the user agent, or whatever it takes
    // to detect that.
    return ...;    
  }

The existence in the request of the JSESSIONID header with the same value that the JSESSIONID cookie demonstrates that the request comes from someone that was able to read the value of the JSESSIONID cookie or the JSESSIONID response header. In a browser that hasn't been compromised this means that only the application that received the original legitimate response. This is thanks to the "same origin policy" of the browser.

The proposal is in fact a variant of the "double submit cookies" technique.

(In reply to Juan Hernández from comment #6)
> The proposed fix doesn't disable per request authentication. The check is
> only performed if the request contains the JSESSIONID cookie, and this
> cookie won't be contained if the caller is using per request authentication.

indeed, so if one uses per request authentication (does not ask for persistent-auth and never got back JSESSIONID cookie in response from api, can't send it in the following request to api) and as result all his requests will be insecure (cause they does not have JSESSIONID http header),

if to address this, he'll have to add persistant-auth:true header, engine will be fluded with zombie sessions which none needs, what will become an abuse of persistent authentication.

anyway per-request auth will be broken after your patch for all clients,
cause they can't fulfill you request of supplying JSESSIONID header.

when i say "disables per-request auth", i mean it's not physically disabled,
but won't be served anymore as is.

> These are the first few lines of the doFilter method:
> 
>    // Nothing to check if there isn't a session yet:
>    HttpSession session = request.getSession(false);
>    if (session == null) {
>      chain.doFilter(request, response);
>      return;
>    }
> 
> The fix does break backwards compatibility, that I agree, but so will do any
> mechanism that requires new headers/parameters/cookies. In my opinion there
> is no way around that. And that is why we need a way to allow users, as a
> last resort, to mark their installation as insecure and thus continue using
> it. 

i don't think this is a right approach, of course you can say it was insecure
before anyway, but my complain is not about being secure or not, but about a
need in doing something in order to be able continue working after this patch, and this is a huge problem,

i think such changes should come in api-v2 and force api-v1 deprecation, but not
breaking v1, it's simply not right.

(btw the fact that you've addressed it in our clients, does not mean you sollved the issue this fix produces ...)

Hi guys, I would like to share some of my opinions on this topic.

> The existence in the request of the JSESSIONID header with the same value
> that the JSESSIONID cookie demonstrates that the request comes from someone
> that was able to read the value of the JSESSIONID cookie or the JSESSIONID
> response header.

+1

In general, the use of cookies is the main source of CSRF vulnerability in web applications. Any kind of user credentials/identification (JSESSIONID cookie, HTTP Authorization header, etc.) that is *automatically* sent by the web browser as part of request essentially opens CSRF vulnerability.

In web applications, web browsers typically *remember* HTTP Basic Auth info once it has been set for given domain (unless you restart the browser) -- this means that also per-request authentication is implicitly subject to CSRF via HTTP Authorization header. I belive there should be a separate patch to address this. (On a side note, we should think about moving away from HTTP Basic Auth altogether, for security reasons.)

Similarly, web browsers automatically send cookies that were set for given domain (JSESSIONID cookie is typically marked as 'session' cookie which means it is deleted on browser restart). As Juan noted, only JavaScript that originated from domain X path Y can read cookie set for domain X path Y. In general, cookies are subject to *both* Same-Origin Policy (same domain) + same path check.

So if the client (JavaScript) is able to read JSESSIONID cookie (and send its value via JSESSIONID *request* header), we can assume that this client can be trusted. In addition to JSESSIONID cookie, REST API backend also sends JSESSIONID *response* header for clients that are on *same origin* but *different path* -- for example:

* WebAdmin GUI runs on http://example/ovirt-engine/webadmin
  -> domain http://example/
  -> path /ovirt-engine/webadmin

* REST API backend runs on http://example/ovirt-engine/api
  -> domain http://example/
  -> path /ovirt-engine/api

As you can see in example above, domains are the same, but paths are *not* the same, this is why having JSESSIONID *response* header is useful for JavaScript clients -- they normally wouldn't be able to read JSESSIONID cookie (due to same-domain + same-path restriction).

Regardless of how the client reads the JSESSIONID value (from cookie or from response header), the important thing is that the client needs to include JSESSIONID *request* header with this value -- this is something web browser won't do automatically, therefore it serves as CSRF request token.

> indeed, so if one uses per request authentication (does not ask for
> persistent-auth and never got back JSESSIONID cookie in response from api,
> can't send it in the following request to api) and as result all his
> requests will be insecure (cause they does not have JSESSIONID http header),

In web browser, requests using per-request authentication will be vulnerable to CSRF anyway, because per-request authentication relies on HTTP Basic Auth -- as I mentioned above, web browsers typically remember Basic Auth info once it has been set for given domain (this is analogous to browsers automatically sending cookies for given domain once the cookie has been set).

As I wrote above, there should be patch to fix CSRF due to HTTP Basic Auth (per-request authentication scenario) as well.

I agree with Juan that there is no way to make CSRF-protection changes *and* stay compatible with old clients at the same time. It's simply because the current authentication mechanism (relying on HTTP Basic Auth and cookies) is vulnerable to CSRF on its own.
 
> anyway per-request auth will be broken after your patch for all clients,
> cause they can't fulfill you request of supplying JSESSIONID header.

We should address CSRF for both per-request authentication (HTTP Authorization header) and persistent authentication (cookie) scenarios.

> i don't think this is a right approach, of course you can say it was insecure
> before anyway, but my complain is not about being secure or not, but about a
> need in doing something in order to be able continue working after this
> patch, and this is a huge problem,
> 
> i think such changes should come in api-v2 and force api-v1 deprecation, but
> not
> breaking v1, it's simply not right.

This is a good point, but it would mean that api-v1 is vulnerable to CSRF whereas api-v2 contains CSRF protection. It's essentially a sacrifice of security in favor of backwards compatibility.

(In reply to vszocs from comment #8)
> > i don't think this is a right approach, of course you can say it was insecure
> > before anyway, but my complain is not about being secure or not, but about a
> > need in doing something in order to be able continue working after this
> > patch, and this is a huge problem,
> > 
> > i think such changes should come in api-v2 and force api-v1 deprecation, but
> > not
> > breaking v1, it's simply not right.
> 
> This is a good point, but it would mean that api-v1 is vulnerable to CSRF
> whereas api-v2 contains CSRF protection. It's essentially a sacrifice of
> security in favor of backwards compatibility.

we are not in position to decide that, but the consumers of our api are,
we should let them being able to choose - this is common practice btw.

I have to insist that non persistent authentication isn't affected in any way by this change, as when there isn't a session the filter does nothing. Callers that use non persistent authentication will work exactly as they used to.

Letting the consumers of the API decide is the reason for the existence of the trust script. The administrators of the engine can use it to decide, with a fine grained criteria. For example, an administrator may decide that he wants to require CSRF protection for requests coming for browsers, but not for requests coming from his curl scripts:

  function isTrusted(request) {
    return request.getHeader("User-Agent").startsWith("curl/");
  }

Or he may decide to give their users one month to update their applications:

  function isTrusted(request) {
    var now = Date();
    var limit = Date(2014, 06, 01);
    return now < limit;
  }

Or he may decide that he doesn't want to sacrifice compatibility at all:

  function isTrusted(request) {
    return false;
  }

Of he may decide that security is paramount (this is the default):

  function isTrusted(request) {
    return true;
  }

Or he may have any other criteria that we aren't aware of. The decision to when to sacrifice security in favor of compatibility is thus in the hands of the administrator, not in our hands.

Juan,

(In reply to Juan Hernández from comment #10)
> I have to insist that non persistent authentication isn't affected in any
> way by this change, as when there isn't a session the filter does nothing.
> Callers that use non persistent authentication will work exactly as they
> used to.

you don't have to insist, but to explain yourself better in the description
of the feature which is says at [1]:

FEATURE-DOC: "Currently we don't have any mechanism to protect the RESTAPI from CSRF attacks. This patch adds a filter that increases the protection from
this attacks requiring callers to include a copy of the session
identifier in a request header. This way only applications that have
legitimately obtained a session cookie will be able to send valid
requests, Requests that don't contain this JSESSIONID header will be rejected with error code 403 (forbidden)"

[1] http://gerrit.ovirt.org/#/c/26578/2//COMMIT_MSG

as i read this, don't have jsession+header -> not get served and "non persistent authentication" users never have jessionid in request, thus
applying basic transitivity logic -> they never get served.

> 
> Letting the consumers of the API decide is the reason for the existence of
> the trust script. The administrators of the engine can use it to decide,
> with a fine grained criteria. For example, an administrator may decide that
> he wants to require CSRF protection for requests coming for browsers, but
> not for requests coming from his curl scripts:
> 
>   function isTrusted(request) {
>     return request.getHeader("User-Agent").startsWith("curl/");
>   }
> 
> Or he may decide to give their users one month to update their applications:
> 
>   function isTrusted(request) {
>     var now = Date();
>     var limit = Date(2014, 06, 01);
>     return now < limit;
>   }
> 
> Or he may decide that he doesn't want to sacrifice compatibility at all:
> 
>   function isTrusted(request) {
>     return false;
>   }
> 
> Of he may decide that security is paramount (this is the default):
> 
>   function isTrusted(request) {
>     return true;
>   }
> 
> Or he may have any other criteria that we aren't aware of. The decision to
> when to sacrifice security in favor of compatibility is thus in the hands of
> the administrator, not in our hands.

that's what i've told to begin with, but fixing things after it got broken using your workaround can't be call "choice" as it already stopped working ...

so after upgrading to latest build (including your change), they will have no choice whatsoever till administrator reads your documentation (let's assume he finds it ...) described here [1] understand what we're talking about / google it a bit, and maybe fix the env,

(don't assuming that all sysadmins on a same level of knowledge as you are,
also sysadmins maybe not in position to decide if it should be trusted or not
so they have to get developer/s involved while they may have used outsourcing to develop the app, etc., to make a long story short - it's a mess),

putting myself in position of product consumer, i'd be really frustrated dealing
with this change without heads up,

thus i'm convinced this is a good change, but for api-v2.

[1]

FEATURE-DOC: "This change will break backwards compatibility with old callers, as they aren't aware of the new requirement. To handle that the filter uses a
configurable script that decides if a request is trusted. Trusted
requests don't need to use the JSESSIONID header. This script doesn't
exist by default, so no request will be trusted by default. In order to
use it the administrator will have to create it."

http://gerrit.ovirt.org/#/c/26578/2//COMMIT_MSG

(In reply to Michael Pasternak from comment #11)
> Juan,
> 
> (In reply to Juan Hernández from comment #10)
> > I have to insist that non persistent authentication isn't affected in any
> > way by this change, as when there isn't a session the filter does nothing.
> > Callers that use non persistent authentication will work exactly as they
> > used to.
> 
> you don't have to insist, 

though looking at the code you indeed ignoring requests with no session,
so i apologize for being lazy & not checking the code first, i think it worth
mentioning that in the commit message/documentation btw,

anyway my point made at comment 11 is still valid.

thanks.

Juan,

i believe there is a way out, If you you want this feature in this version of api, you can push it, but make it disabled by default (so no clients will stop
working), this way:

1. users can adapt to it
2. UI can start using it imminently
3. you can state it will be forced at api-v2

how does it sounds?

(In reply to Michael Pasternak from comment #13)
> Juan,
> 
> i believe there is a way out, If you you want this feature in this version
> of api, you can push it, but make it disabled by default (so no clients will
> stop
> working), this way:
> 
> 1. users can adapt to it
> 2. UI can start using it imminently
> 3. you can state it will be forced at api-v2
> 
> how does it sounds?

This would be acceptable from a security perspective. The initial patch with an option to enable this feature would be shipped as an RHSA, with documentation explaining that manual configuration is necessary to apply the fix.

The CSRF protection filter has been reworked so that it is disabled by default. It can be enabled using the "CSRFProtection" configuration parameter:

  # engine-config -s CSRFProtection=true

Then the engine needs to be restarted.

In addition the clients can decide to enable/disable protection using the "Prefer" header. If the first request of the session contains the "Prefer" header with the "csrf-protection" element then protection will be enabled, otherwise it will be disabled. This is intended for clients that don't need CSRF protection, as they don't run inside a browser (the SDKs and the CLI, in particular).

More details in the commit message of the change:

  http://gerrit.ovirt.org/29681

The GUI applications do run in a browser, and I'm proposing to modify them so that they always send the "Prefer" header requesting protection:

  http://gerrit.ovirt.org/29682

Note that once protection is enabled (with the CSRFProtection parameter) all UI plugins that use the session identifier passed by webadmin will need also to pass the session identifier in the "JSESSIONID" header.

Is there any chance to deprecate the old behaviour for UI-Plugins in some
future version?

As far as I know there are not a heck of a lot UI plugins, couldn't those
few devs get encouraged to adopt this new parameter?

What I am aiming for is a secure default installation.

The default installation has no plugins installed (correct my if I'm wrong).

The default installation of oVirt doesn't have plugins installed, so you can perfectly change the CSRFParameter to true right after installation, if you want to have a more secure installation.

I have already notified the plugin developers I know. If the change is accepted we will notify other plugin developers.

For the future we plan to change the session tracking mechanism. Currently we use a JSSESSIONID cookie, which is problematic for JavaScript applications. In the future we want to use the JSESSIONID instead. This means that all client applications, including UI plugins, will have to send the JSESSIONID header. Once they do this change, they will be automatically supported by the CSRF protection mechanism. This will probably happen with oVirt 4.x, but there isn't a commitment yet.

(In reply to Juan Hernández from comment #15)

> The GUI applications do run in a browser, and I'm proposing to modify them
> so that they always send the "Prefer" header requesting protection:

+1

> Note that once protection is enabled (with the CSRFProtection parameter) all
> UI plugins that use the session identifier passed by webadmin will need also
> to pass the session identifier in the "JSESSIONID" header.

UI plugins that wish to integrate with Engine REST API will be encouraged to utilize oVirt.js library (which will take care of HTTP communication details).

More generally, UI plugins shouldn't make any assumptions about "CSRFProtection" config parameter value. If a UI plugin wishes to talk directly to Engine REST API (i.e. using XMLHttpRequest), it must take care of sending "JSESSIONID" request header on its own.

(In reply to Sven Kieske from comment #16)

> As far as I know there are not a heck of a lot UI plugins, couldn't those
> few devs get encouraged to adopt this new parameter?

UI plugin devs will be encouraged to use oVirt.js.


(In reply to Juan Hernández from comment #17)

> For the future we plan to change the session tracking mechanism. Currently
> we use a JSSESSIONID cookie, which is problematic for JavaScript
> applications. In the future we want to use the JSESSIONID instead. This
> means that all client applications, including UI plugins, will have to send
> the JSESSIONID header. Once they do this change, they will be automatically
> supported by the CSRF protection mechanism.

This is a *much* needed improvement, at least from UI (plugin) point of view.

@Juan, just out of curiosity, after we support "JSESSIONID" request/response header as an option for session tracking mechanism, can't we remove "Prefer:csrf-protection" and simply honor "CSRFProtection" config parameter?

(In reply to vszocs from comment #18)
> @Juan, just out of curiosity, after we support "JSESSIONID" request/response
> header as an option for session tracking mechanism, can't we remove
> "Prefer:csrf-protection" and simply honor "CSRFProtection" config parameter?

We can remove "Prefer: csrf-protection" once that all clients are moved to use the JSESSIONID header instead of the cookie, but that will be a long time.

(In reply to Juan Hernández from comment #19)
> We can remove "Prefer: csrf-protection" once that all clients are moved to
> use the JSESSIONID header instead of the cookie, but that will be a long
> time.

OK, sounds reasonable, thanks for your answer.

oVirt 3.5 has been released and should include the fix for this issue.