Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encrypt access tokens #90

Closed
wants to merge 1 commit into from
Closed

Encrypt access tokens #90

wants to merge 1 commit into from

Conversation

micbar
Copy link
Member

@micbar micbar commented Oct 5, 2022

Description

We need to encrypt our JWTs before we send them to the Web Application.

Spec

https://www.rfc-editor.org/rfc/rfc7516

To do

  • Implement Key generation
  • Implement Encrypt/Decrypt
  • Add config switch
  • Add Documentation
  • Test with invalid tokens

Config

This can be enabled in the wopiserver.conf

# location of the secret files. Requires a restart of the
# WOPI server when either the files or their content change.
wopisecretfile = /etc/wopi/wopisecret
iopsecretfile = /etc/wopi/iopsecret
jwesecretfile = /etc/wopi/jwesecret
encryptjwetoken = True

Outcome

Payload is encrypted, headers are readable

1664985777406

@micbar micbar requested review from glpatcern and javfg as code owners October 5, 2022 15:53
@micbar micbar marked this pull request as draft October 5, 2022 15:54
@wkloucek
Copy link
Contributor

wkloucek commented Oct 5, 2022

The reasoning is that the REVA access tokens is send inside the WOPI JWT token to following services:

  • Office Application
  • Client Browser
  • depending on deployment: reverse proxy

The REVA access token is only intended to be used by the WOPI server and should never be exposed to 3rd party services.

@glpatcern
Copy link
Member

The REVA access token is only intended to be used by the WOPI server and should never be exposed to 3rd party services.

This PR implements an interesting feature indeed, thanks. But I dare challenging this statement: AFAIU currently the web UI holds the Reva access token as well, so it does not seem it is meant for backend services only or for limited distribution - clients have a way to hold it and reuse it as they like. Also, if we were to restrict the WOPI server to not include the Reva token within its own token, the WOPI server would become a stateful service requiring a central key/value store for the Reva tokens. IMHO this would over-complexify the service, for relatively little added benefit (WOPI traffic is https as well as client browser traffic).

@wkloucek
Copy link
Contributor

wkloucek commented Oct 5, 2022

The REVA access token is only intended to be used by the WOPI server and should never be exposed to 3rd party services.

This PR implements an interesting feature indeed, thanks. But I dare challenging this statement: AFAIU currently the web UI holds the Reva access token as well, so it does not seem it is meant for backend services only or for limited distribution - clients have a way to hold it and reuse it as they like. Also, if we were to restrict the WOPI server to not include the Reva token within its own token, the WOPI server would become a stateful service requiring a central key/value store for the Reva tokens. IMHO this would over-complexify the service, for relatively little added benefit (WOPI traffic is https as well as client browser traffic).

ownCloud Web uses an OIDC access token (also JWT for most OIDC providers) - but never a REVA JWT token. These OIDC access tokens are typically short lived.

We still transport the REVA access token inside the WOPI JWT token to prevent the WOPI server becoming a stateful application. But since the token is now encrypted, only the WOPI server can inspect it.

@micbar
Copy link
Member Author

micbar commented Oct 5, 2022

We still transport the REVA access token inside the WOPI JWT token to prevent the WOPI server becoming a stateful application. But since the token is now encrypted, only the WOPI server can inspect it.

Correct. This adds no more state to the wopiserver. If we scale the wopiserver into multiple ones, we need to share the config volume with the secrets. But that was already the case for the wopisecret and the iopsecret

Difference between OIDC tokens and Reva Access Tokens

Web token payload

{
  "aud": "web",
  "exp": 1665046243,
  "jti": "B13TElT2QKAw58AhEswPfZxV34skINBn",
  "iat": 1664959843,
  "iss": "https://localhost:9200",
  "sub": "vn047YDEoJdbFHQ7@x0o8AjgeB8vaEK8M5rD1nIR359aUVKRXsY8LSSOCzV_8rJwROifQQAn9PxBxgomJlmPNEQ",
  "lg.t": "1",
  "scp": "profile email openid",
  "lg.i": {
    "dn": "Admin",
    "id": "uid=admin,ou=users,o=libregraph-idm",
    "un": "admin"
  },
  "lg.p": "identifier-ldap"
}

Reva Access Token payload

{
  "appediturl": "http://localhost:8090/hosting/wopi/word/edit",
  "appname": "OnlyOffice",
  "appviewurl": "http://localhost:8090/hosting/wopi/word/view",
  "endpoint": "1284d238-aa92-42ce-bdc4-0b0000009157$some-admin-user-id-0000-000000000000",
  "exp": 1665067872,
  "filename": "some-admin-user-id-0000-000000000000/Neue Datei (1).docx",
  "folderurl": "https://localhost:9200/f/1284d238-aa92-42ce-bdc4-0b0000009157$some-admin-user-id-0000-000000000000!f42aefa3-0f90-4d21-8ade-95b0c8658383",
  "iss": "cs3org:wopiserver:git",
  "userid": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJhdWQiOiJyZXZhIiwiZXhwIjoxNjY1MDY3ODcyLCJpYXQiOjE2NjQ5ODE0NzIsImlzcyI6Imh0dHBzOi8vbG9jYWxob3N0OjkyMDAiLCJ1c2VyIjp7ImlkIjp7ImlkcCI6Imh0dHBzOi8vbG9jYWxob3N0OjkyMDAiLCJvcGFxdWVfaWQiOiJzb21lLWFkbWluLXVzZXItaWQtMDAwMC0wMDAwMDAwMDAwMDAiLCJ0eXBlIjoxfSwidXNlcm5hbWUiOiJhZG1pbiIsIm1haWwiOiJhZG1pbkBleGFtcGxlLm9yZyIsImRpc3BsYXlfbmFtZSI6IkFkbWluIiwiZ3JvdXBzIjpbIjUwOWE5ZGNkLWJiMzctNGY0Zi1hMDFhLTE5ZGNhMjdkOWNmYSJdLCJ1aWRfbnVtYmVyIjo5OSwiZ2lkX251bWJlciI6OTl9LCJzY29wZSI6eyJ1c2VyIjp7InJlc291cmNlIjp7ImRlY29kZXIiOiJqc29uIiwidmFsdWUiOiJleUp3WVhSb0lqb2lMeUo5In0sInJvbGUiOjF9fX0.eMVvQJRl_4P8tRWqQYZfhejcBHUyS2Ct5Z-GDq-AxEI",
  "username": "Admin",
  "viewmode": "VIEW_MODE_READ_WRITE",
  "wopiuser": "some-admin-user-id-0000-000000000000@https://localhost:9200"
}

Payload the userid which is in fact a Reva Token

{
  "aud": "reva",
  "exp": 1665067872,
  "iat": 1664981472,
  "iss": "https://localhost:9200",
  "user": {
    "id": {
      "idp": "https://localhost:9200",
      "opaque_id": "some-admin-user-id-0000-000000000000",
      "type": 1
    },
    "username": "admin",
    "mail": "[email protected]",
    "display_name": "Admin",
    "groups": [
      "509a9dcd-bb37-4f4f-a01a-19dca27d9cfa"
    ],
    "uid_number": 99,
    "gid_number": 99
  },
  "scope": {
    "user": {
      "resource": {
        "decoder": "json",
        "value": "eyJwYXRoIjoiLyJ9"
      },
      "role": 1
    }
  }
}

@micbar
Copy link
Member Author

micbar commented Oct 5, 2022

@glpatcern @C0rby @wkloucek I think this is an important gain in security.

@glpatcern
Copy link
Member

@micbar as mentioned I approve this feature, there's no question. I'm just claiming that there's not such security gain as the surface attack is very small: either the client needs to be compromised (and then all bets are off, an attacker would have much more juice to look out in the browser's cache than the WOPI token...), or the application endpoint needs to be compromised (which I guess we must exclude, but consequences would be much worse as the whole content of users' documents gets exfiltrated anyway!).

Also, another option worth considering in the future is to downscope the Reva token to the area of interest for WOPI operations - essentially read/write on the file and potentially other files in the same folder, nothing else.

BTW I actually assumed we expose the Reva token on web and I now realize this is not the default, but it's what we're doing and we're even going one step further as you've seen on cs3org/reva#3315, for as important usability reasons. More details on that will come from @labkode.

@@ -619,6 +621,21 @@ def cboxDownload_deprecated():
return iopDownload()


def initEncryptionKey(cls):
try:
with open(cls.config.get('security', 'jwesecretfile'), 'r') as kf:
Copy link
Member

Choose a reason for hiding this comment

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

A minor comment: for the secret we can safely reuse the already configured wopisecretfile value, which is only used by the wopiserver (not shared with Reva) to encode the JWT. In other words the same secret either encodes or encrypts depending on the config flag.

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to be an AES256 keywrap.

{"k":"P6mA6K6Jsaob6-ch7krZTacV9VRE7Xx58bZ8vbkV6Lw","kty":"oct"}

Copy link
Member

Choose a reason for hiding this comment

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

I see. But shouldn't JWE with direct encryption work as well? The payload is not that large IMHO to require the extra burden of using JWK with the key-encrypting key inside. But I'm not used to typical practices for JWE or JWK tokens so please take it with a grain of salt ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me check again. I tried it with a plain "key_from_password" approach, but the byte length was always mismatching.

@micbar
Copy link
Member Author

micbar commented Oct 6, 2022

BTW I actually assumed we expose the Reva token on web and I now realize this is not the default, but it's what we're doing and we're even going one step further as you've seen on cs3org/reva#3315, for as important usability reasons. More details on that will come from @labkode.

Seen it. See my comment. IMO we should never work with reva tokens on clients.

@wkloucek
Copy link
Contributor

wkloucek commented Oct 6, 2022

... or the application endpoint needs to be compromised (which I guess we must exclude, but consequences would be much worse as the whole content of users' documents gets exfiltrated anyway!).

Also, another option worth considering in the future is to downscope the Reva token to the area of interest for WOPI operations - essentially read/write on the file and potentially other files in the same folder, nothing else.

In some cases like SaaS Web Office (eg. Office 365) the attack surface is unknown and out of your control. So one should better not give them credentials, they do not need.

Exactly, the REVA token should be scoped. But also the WOPI token is already scoped to the current document, so we would be fine for the beginning.

@C0rby
Copy link

C0rby commented Oct 12, 2022

I think this concept is good.

In some cases like SaaS Web Office (eg. Office 365) the attack surface is unknown and out of your control. So one should better not give them credentials, they do not need.

This also increases the difficulty of attacks via macros in documents.
As of right now, it's possible to get access to a users reva token by sharing a document containing a macro.
Sure there is a warning when opening those documents but as we saw in the past documents containing macros is a widely used and working attack surface.

@glpatcern
Copy link
Member

glpatcern commented Oct 12, 2022

This also increases the difficulty of attacks via macros in documents.
As of right now, it's possible to get access to a users reva token by sharing a document containing a macro.

@C0rby can you please elaborate on this? Are you saying that there's a known flaw in Office 365 such that the WOPI token gets exposed if a document contains a (specially-crafted, I guess) macro?

@C0rby
Copy link

C0rby commented Oct 12, 2022

I wasn't been able to test this in Office365 yet but at least on OnlyOffice there is the option to add macros to a document under the plugins tab.
image
These are JavaScript macros. This code for example is able to print the download url of the current document to the browser console:

(function()
{
    console.log(parent.fileInfo.DownloadUrl);
})();

This download url also contains the wopi token (which contains the reva token) as a query parameter.

If I, as an attacker, would want to get the tokens of other people I could just send this download url to a server under my control instead of printing it to the console.

Whether this is a flaw or not depends on the person you ask, I guess. Macros are there to allow the user to execute arbitrary code. So this works as designed.

I would like to test if something like this works on Office365 but currently in our demo instance I'm unable to open a document there.

@glpatcern
Copy link
Member

This may be considered a serious flaw on the macro engine in OnlyOffice, and the fact that the WOPI token is exposed (regardless the Reva token being encrypted) is already a sufficient threat - as I mentioned, with that any document can easily be exfiltrated or deleted.

Yet, as an attacker, you'd need to get control over the client's browser to be able to execute that macro. Otherwise how would you force a client to execute your macro?

@C0rby
Copy link

C0rby commented Oct 14, 2022

Yet, as an attacker, you'd need to get control over the client's browser to be able to execute that macro. Otherwise how would you force a client to execute your macro?

The attacker could just share a document with a macro to users of the instance.
Yes, this means the attacker needs to have an account or access to an account of an oCIS deployment and the sharees need to open the document.

But both situations are not impossible.

@glpatcern
Copy link
Member

OK, so we're back to a "traditional" phishing attack. Surely not to be neglected, but the attack surface is not getting significantly larger. IMHO once an attacker gets access to an organization, there's plenty of easier penetration paths than this to leverage.

@glpatcern glpatcern force-pushed the master branch 4 times, most recently from 84ad495 to bfe5ddc Compare December 12, 2022 09:32
@glpatcern glpatcern force-pushed the master branch 2 times, most recently from bb4647b to 95d687f Compare February 19, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants