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

Sensitive information are being persisted in local storage #3436

Open
jiachen247 opened this issue Jun 12, 2019 · 56 comments
Open

Sensitive information are being persisted in local storage #3436

jiachen247 opened this issue Jun 12, 2019 · 56 comments
Assignees
Labels
Auth Related to Auth components/category Cognito Related to cognito issues needs-discussion Used for internal discussions Security Issues related to security concerns Service Team Issues asked to the Service Team

Comments

@jiachen247
Copy link

Describe the bug
All cognito session tokens id, access and refresh tokens are being persisted into localstorage. This goes against all industry security best practice of storing sensitive infomation in signed httponly cookies.

To see why its bad practice this article presents a summary.

Also known as Offline Storage, Web Storage. Underlying storage mechanism may vary from one user agent to the next. In other words, any authentication your application requires can be bypassed by a user with local privileges to the machine on which the data is stored. Therefore, it's recommended not to store any sensitive information in local storage.

@haverchuck haverchuck added this to the Auth v2 milestone Jun 12, 2019
@haverchuck haverchuck added needs-discussion Used for internal discussions Auth Related to Auth components/category to-be-reproduced Used in order for Amplify to reproduce said issue and removed needs-discussion Used for internal discussions labels Jun 12, 2019
@haverchuck haverchuck removed the to-be-reproduced Used in order for Amplify to reproduce said issue label Jun 17, 2019
@stale stale bot closed this as completed Jul 24, 2019
@jacintoArias
Copy link

Is this being ignored? or is there somewhere we can track the status of this?

@mlabieniec mlabieniec added Security Issues related to security concerns and removed pending-close-response-required labels Sep 12, 2019
@mlabieniec mlabieniec reopened this Sep 12, 2019
@aws-amplify aws-amplify deleted a comment from stale bot Sep 12, 2019
@aws-amplify aws-amplify deleted a comment from stale bot Sep 12, 2019
@mlabieniec
Copy link
Contributor

mlabieniec commented Sep 12, 2019

We are evaluating this. This argument can be ambiguous as well. For example see thread here: https://www.reddit.com/r/Frontend/comments/cubcpj/local_storage_vs_cookies_for_auth_tokens/ in addition to the comments/discussion in the above thread you referenced.

However, it's an interesting problem in this particular space (client-side/browser), with the ultimate solution of simply not storing / using a persistent session (only storing in memory, which you can do with the SDK currently). Obviously, the down-side to this is needing to login in every time the page is refreshed. This issue exists storing in any storage medium i.e. localstorage, cookies / even httpOnly; even though JS can't access httpOnly tokens, this cookie is still sent with every request, so if JS is injected into your app (considering the main argument on this is generally XSS), API requests will technically still be made as an authenticated user.

That said, the underlying Cognito SDK is using temporary credentials, these credentials need to be refreshed and Signature V4 signed in order to make authenticated AWS API calls. Also, these credentials are scoped to your authenticated/unauthenticated identities. So essentially, if someone was to first access your computer, and get into your web browser, they would need to create a signed request using an AWS SDK with your secret key and access key before the credentials have expired. You can also invalidate these credentials in the event of this via the IAM console.

However, it's still an ambiguous topic with problems in all areas. We will be leaving this issue open to track feedback on this as we evaluate the path forward and most secure approach available when utilizing a client-side / serverless architecture with amplify.

@jacintoArias
Copy link

Hi, thanks for reopening.

I agree that the opinions and strategies here are not universally valid.

In my case I am using amplify to manage authentication with cognito by just retrieving and using Access Bearer tokens to call API Gateway with cognito authorizers, so no IAM temporary credentials for me.

However the access token is temporary as well and the same principle applies (the attacker could use the token just until it expires). My main concern is the refresh token, which has a much longer expiration time and can be used to reclaim fresh credentials until the attacker is spotted and the token are revoked.

I will be much more confident by following a strategy similar to the silent authentication implemented in Auth0, or knowing that there is a good reason to have a refresh token in the localStorage of my angular SPAs.

https://auth0.com/docs/tokens/refresh-token/current

Otherwise, can anyone confirm if implementing the Authorization code grant pattern leverages the problem of storing such refresh token?

@mlabieniec
Copy link
Contributor

Ah i see, Assuming you are referring to this as to what you are using?
https://docs.aws.amazon.com/apigateway/latest/developerguide/apigateway-integrate-with-cognito.html

The temporary credentials I am referring to are the ones that are signing all your AWS requests i.e. secret key and access key. Are you using the Amplify Auth Category, initialized by the CLI with your project, as in, are the services (API Gateway, Cognito etc...) in your application using authenticated / unauthenticated IAM roles?

@jacintoArias
Copy link

Exactly, we are providing a manual cognito configuration to the amplify SDK for angular and we are using such cognito user pool as an authorizer for our APIs (both using direct integration and lambda authorizers depending on the microservice)

We use just the user pool with an OAuth Authorization Code Grant, so amplify redirects to the hosted UI and parses the corresponding callback query param, it is already very nifty compared with our previous implementation without amplify.

@mlabieniec
Copy link
Contributor

mlabieniec commented Sep 12, 2019

Ah ok understood now thanks! So, you are not using federated identities at all and user pools directly. I was actually thinking that potentially the lambda authorizer could be an interesting solution in this case, even to potentially add a layer of security. We are working on some authentication work now that's still in the design phase, so this is a good thread to add to these discussions, will track/keep this up-to-date here as we move along, thanks for the details.

@mlabieniec
Copy link
Contributor

@jacintoArias do you have device tracking turned on in Cognito User Pools? This will actually add additional security to the refresh token which it will only work on the specific device: https://docs.aws.amazon.com/cognito/latest/developerguide/amazon-cognito-user-pools-device-tracking.html

@jacintoArias
Copy link

Tracking the device will definitely improve the security of the refresh token and fits in our requirements, thanks for the recommendation we will give it a try.

We are currently migrating from a lambda authorizer to directly integrating with cognito beacuse we precisely think that cognito's implementation of token validation could be more advanced than our own custom token validation. Basic authorization is being done by filtering the endpoints using oauth claims in the token.

I will be very interested in knowing if If you think that a lambda authorizer is a better and more secure approach than leveraging the cognito integration directly...

@mlabieniec
Copy link
Contributor

Great to hear! Yes i think the device tracking is a good solution here. For the authorizer i think the Cognito one is good for what you are doing, i was more referring to a possible solution where perhaps we could utilize to add a layer of additional validation, however the device tracking does a good job of locking down the refresh token per device.

@Jun711
Copy link

Jun711 commented Sep 27, 2019

@mlabieniec
I wonder how accurate device tracking is. Is there an easy way for users to reset the tracking id?

@evbo
Copy link

evbo commented Oct 1, 2019

What about SPA use-cases, requiring a user to stay logged in greater than 1 hour but no more than 8 hours?

Would the Cognito team (I presume amplify works closely with them) take as feedback to please allow either:

  • refresh token expiration less than 1 day (so that if stolen has very limited TTL)
  • access token expiration at most 8 hours (obviates the need for refresh token, but still limited TTL)

Either of the above enhancements would be arguably safer since it limits the TTL of the very sensitive refresh token information.

Secondary concerns that compound vulnerability:

  • amplify is javascript, so it can't set http-only cookies and non http-only cookies are especially prone to XSS
  • Cognito doesn't support implicit grant type or PKCE, forcing SPA implementations to store refresh token in local storage or cookies
  • refresh tokens are very big, such that network request payloads are more demanding to handle if malicious.

Similar security concerns expressed here:
#3224
#1218
#3774
#2213
#1735

@mlabieniec
Copy link
Contributor

mlabieniec commented Oct 1, 2019

@Jun711 If you delete local storage, a new device ID will get generated.

@evbo will look into this for you and get some details from the Cognito team on these requests/concerns.

@evbo
Copy link

evbo commented Oct 3, 2019

@mlabieniec thank you.

Just wanted to emphasize that my second suggestion: increasing access token expire is really the preferred solution in lieu of silent auth features being supported.

Here's some additional useful context for the Cognito team. I hope they receive this feedback well:

  1. The security issue at hand is well stated here: Refresh access and id tokens in a React/Angular SPA amazon-archives/amazon-cognito-auth-js#92

  2. Configuring access token expire is inline with the spec (there is no documented min/max required): https://tools.ietf.org/html/rfc6749#section-4.2.2

  3. Configurable access token expire for Cognito is a fairly popular discussion on SO/forums as well:

@mlabieniec
Copy link
Contributor

@evbo I have discussed with the cognito team this feature and they have informed me that this is in their backlog as a feature. I do not have a completion date on this yet tho will keep this issue open to track progress from their side.

@mlabieniec mlabieniec added Cognito Related to cognito issues Service Team Issues asked to the Service Team labels Oct 4, 2019
@ravenscar
Copy link

@michaelbrewer this guide looks sensible but it is suggesting features such as silent refresh, which cognito doesn't have. A lot of the decisions would make you change from the Cognito UI as well which will mean you lose access to a bunch of OAuth stuff like code flow etc, so if using Cognito you will have to reimplement code flow using your own server side crypto and storage.

What I am saying is that I don't think you can follow the practices of this guide, or good practices in general, whilst using Cognito and/or Amplify.

@rspuler73
Copy link

rspuler73 commented Oct 12, 2021

I have to agree with @ravenscar. Much of the OAuth/OIDC stuff is only provided with the Cognito Hosted UI which is pretty useless in any customer facing application as it is very limited in what can be customized (e.g. lack of i18n support). So there is no way to use it as an OIDC without implementing everything yourself.

@dittmarconsulting
Copy link

dittmarconsulting commented Oct 25, 2021

Luckily I found this thread as I was scratching my head why I could read the payload of the access token with this one-liner:

console.log(base64.decode(token.split('.')[1]))

I get something like that (modified)

{"kid":"IDq0iinUEq4\/yp4PfebvmcShx1JSDz2fH96iKMVJumE=","alg":"RS256"}{"sub":"00757755-3cbc-46e6-9212-2510b86b5e1e","event_id":"6bc5abff-9u79-4036-af26-964bed08a6da","token_use":"access","scope":"aws.cognito.signin.user.admin","auth_time":1635149829,"iss":"https:\/\/cognito-idp.ap-southeast-2.amazonaws.com\/ap-southeast-2_7rJr7xrje","exp":1635153429,"iat":1635149829,"jti":"1w2a054e-bef8-402c-8509-2cbeed0ef71b","client_id":"5t4p9qpgpn469vs5s97ocsnq8n","username":"00677799-3cbc-67o9-9212-2510b86b5e1e"}

@Antonio-Riccelli
Copy link

Hi. What is the current status on this, please?

@bearsworth
Copy link

I posted for a feature request on a similar note. But moving forward, instead of using async storage, can't you guys just use encrypted storage solutions out there?

@wz2b
Copy link

wz2b commented Jan 28, 2023

The thing I find spooky is that, in addition to the content in the tokens themselves, Auth also stores UserAttributes which has all of your cognito attributes (even the custom ones) in it. So someone hacking the local storage later might only get expired keys, but they also get your name, phone number, e-mail. There's even more information in the idToken. At work we'd call this Personal Identifiable Information (PII) and, like others who made this comment, I think that storing that forever is a problem.

You can avoid this by using sessionStorage (I guess) but like everyone has pointed out that's pretty inconvenient, too.

I kind of feel like there's no good answer to this.

@wz2b
Copy link

wz2b commented Jan 28, 2023

I should add this, though for everyone who is nervous about this: if you call Auth.signOut() the local storage is cleared.

@tannerabread tannerabread changed the title Sensitive infomation are being persisted in local storage Sensitive information are being persisted in local storage Mar 13, 2023
@zuluaica18
Copy link

It is important to emphasize that if your application is going to go through security tests with a company that trusts the owasp definitions, you would have serious problems with the output of your project. Keep that in mind!

It seems to me that aws and owasp have a lot to talk about...

@zuluaica18
Copy link

I would like to see an alternative storage that is cookie httponly , and amplify could create the lambda to manage the oauth flow with the server, I would pay more infrastructure but it would be a beautiful option for some, this would bring the advantage of passing the strict security tests based on owasp from my company. Meanwhile, amplify escapes my use case.

@kevoj7
Copy link

kevoj7 commented Oct 22, 2023

Using nextjs 13.4+, it stores it in cookies now!! I've been playing with it lately and works nicely so far:

This package should do it

Docs: https://docs.amplify.aws/lib-v1/q/platform/js/

@JacobDel
Copy link

JacobDel commented Nov 6, 2023

We are evaluating this. This argument can be ambiguous as well. For example see thread here: https://www.reddit.com/r/Frontend/comments/cubcpj/local_storage_vs_cookies_for_auth_tokens/ in addition to the comments/discussion in the above thread you referenced.

However, it's an interesting problem in this particular space (client-side/browser), with the ultimate solution of simply not storing / using a persistent session (only storing in memory, which you can do with the SDK currently). Obviously, the down-side to this is needing to login in every time the page is refreshed. This issue exists storing in any storage medium i.e. localstorage, cookies / even httpOnly; even though JS can't access httpOnly tokens, this cookie is still sent with every request, so if JS is injected into your app (considering the main argument on this is generally XSS), API requests will technically still be made as an authenticated user.

That said, the underlying Cognito SDK is using temporary credentials, these credentials need to be refreshed and Signature V4 signed in order to make authenticated AWS API calls. Also, these credentials are scoped to your authenticated/unauthenticated identities. So essentially, if someone was to first access your computer, and get into your web browser, they would need to create a signed request using an AWS SDK with your secret key and access key before the credentials have expired. You can also invalidate these credentials in the event of this via the IAM console.

However, it's still an ambiguous topic with problems in all areas. We will be leaving this issue open to track feedback on this as we evaluate the path forward and most secure approach available when utilizing a client-side / serverless architecture with amplify.

I get the argument for storing refresh tokens, but why are the cognito user attributes shown in plain sight?
Why not use local storage for the refresh tokens and cookies for the attributes?

@TeoTN
Copy link

TeoTN commented Jan 1, 2024

Using nextjs 13.4+, it stores it in cookies now!! I've been playing with it lately and works nicely so far:

This package should do it

Docs: https://docs.amplify.aws/lib-v1/q/platform/js/

Not sure how you got to this result, I'm using "aws-amplify": "6.0.9" and I can see all the data in local storage.

EDIT: My bad, I just found out that you can change token-saving mechanism.

@rdsedmundo
Copy link

Although it's possible to switch to a CookieStorage, it doesn't come without its problems.

See:
#1545 (closed without being addressed)
#10498

@hasan-web
Copy link

Any update on this ????

@LydGol90
Copy link

LydGol90 commented Apr 4, 2024

In case it helps anyone, I use react native and we have a similar problem in that the default is to use react native async storage which is not secure. I migrated to ios keychain and android keystore using the following and it seems to be working. We will shortly be getting this re-tested by a pen-test company so I can let you know if it meets their standards. Caveat that the below code is rough and not thoroughly tested yet.

import { Amplify, type ResourcesConfig } from 'aws-amplify';
import { defaultStorage, KeyValueStorageInterface } from 'aws-amplify/utils';
import { cognitoUserPoolsTokenProvider } from 'aws-amplify/auth/cognito';
import {
  getAllGenericPasswordServices,
  getGenericPassword,
  resetGenericPassword,
  setGenericPassword,
} from 'react-native-keychain';

class MyCustomStorage implements KeyValueStorageInterface {
  async setItem(key: string, value: string): Promise<void> {
    await setGenericPassword(key, value, { service: key });
  }
  async getItem(key: string): Promise<string | null> {
    const data = await getGenericPassword({ service: key });
    if (data) {
      return data.password;
    } else {
      // Migrate existing users from old storage to new storage
      const oldData = await defaultStorage.getItem(key);
      if (oldData) {
        await setGenericPassword(key, oldData, { service: key });
        await defaultStorage.removeItem(key);
        return oldData;
      }
    }
    return null;
  }
  async removeItem(key: string): Promise<void> {
    await resetGenericPassword({ service: key });
  }
  async clear(): Promise<void> {
    const storedServices = await getAllGenericPasswordServices();
    const promises = storedServices.map(service =>
      resetGenericPassword({ service }),
    );
    await Promise.all(promises);
  }
}

cognitoUserPoolsTokenProvider.setAuthConfig(authConfig);
cognitoUserPoolsTokenProvider.setKeyValueStorage(new MyCustomStorage());

Amplify.configure(
  {
    Auth: authConfig,
  },
  { Auth: { tokenProvider: cognitoUserPoolsTokenProvider } },
);

@sc0ttdav3y
Copy link

I've also had a negative pen test report recently on Cognito's handling of secrets (specifically the refresh token), but this time I'm on a web application.

I'm experimenting with using a web worker to hide the data as per Auth0's blog post:
https://auth0.com/blog/secure-browser-storage-the-facts/

Here's a gist of an experimental web worker implementation. It works, but still has a race condition to resolve due to the nature of web worker comms being asynchronous.
https://gist.github.com/sc0ttdav3y/55acd1f0d7b9cac3955aaa074e394d7e

From my research, the web worker solution seems to be the only practical way forward in browser-land at present. If this code is useful to anyone then feel free to use it.

The other idea I've got is to simply not store the secret at all. This could be done for the refresh token, but obviously not the access token. To do it, I've been toying with the idea of implementing some form of API Gateway + Lambda solution, where the app would register its refresh token to the server when it first gets it, and then it would call the Lambda via API to rotate its access token, by simply passing its access token and having it all happen server-side and return the new access token.

I hope AWS is reading this. It's an almost 5 year old issue so I hold out no hope, but it's certainly not best practice as it is now.

@sc0ttdav3y
Copy link

One more point to make on this topic. I see there's CookieStorage, and I initially went to use it but then realised that client-generated cookies offers no additional security over Local Storage.

Cookies are only secure if set on the server with the secure and httpsOnly flags.

@gary-archer
Copy link

If it helps anyone, I have an example SPA you can run that integrates with AWS Cognito.

Originally it used Cognito tokens in the browser and I updated it to use the token handler pattern, to store all tokens in HTTP-only SameSite=strict cookies that are issued by utility APIs. This deals with malicious JavaScript threats.

It doesn't use Amplify though, and requires a more complex flow with more moving parts.

@JacobDel
Copy link

JacobDel commented Apr 5, 2024

One more point to make on this topic. I see there's CookieStorage, and I initially went to use it but then realised that client-generated cookies offers no additional security over Local Storage.

Cookies are only secure if set on the server with the secure and httpsOnly flags.

@sc0ttdav3y, I don't understand the difference between storing the refresh token in a cookie (so storing it at your server) vs the implementation above

I'm referring to :

I've been toying with the idea of implementing some form of API Gateway + Lambda solution, where the app would register its refresh token to the server when it first gets it, and then it would call the Lambda via API to rotate its access token

@sc0ttdav3y I'm also confused what the difference is between your web worker and using SessionStorage? Both methods store the data in memory and in both cases the data is lost when the browser is closed, if I understand correctly.

@sc0ttdav3y
Copy link

@JacobDel There's two types of cookie. The one currently implemented with Amplify (CookieStorage) is a cookie set by JS and it's vulnerable to malicious JS, as any cookie set in JS can also be trivially read by any other JS running on the same page via document.cookie. The other type is a server-issued cookie (not implemented by Amplify). This is actually still stored in the browser, but when set with httpOnly its contents cannot be read by JS code.

You also mentioned the SessionStorage implementation — that stores your tokens in plain sight just like local storage, so any JS on the page can trivially hit up your secrets with window.sessionStorage. It has the same risks as client-issued cookies and local storage.

It's different to the web worker solution I linked to because the web worker storage is not accessible by injected JS, while session storage is accessible. However, as it stands now the web worker example I offered is about the same as MemoryStorage, as both offer about the same security but no persistence. Auth0's article explains why web workers are interesting beyond in-memory solutions.

I'm really interested in what @gary-archer has done. His solution sounds like it implements the httpOnly cookie by using a utility server to issue it. This is similar to my own server-side thinking, however, I'm trying to remain within the Amplify SDK if I can so I'm looking to do everything through a custom storage adapter. My thinking is to keep the access token stored as-is in local storage but offload the refresh token to a server endpoint, which would be protected via the access token (i.e. Cognito + API Gateway + Lambda). With some polling, I can keep the access token refreshed without storing the refresh token itself to the JS. And by doing it in a custom storage adapter, Amplify's SDK will continue to work.

It's certainly an interesting conversation. Perhaps others subscribed to this issue have had the past 5 years to think of better ways than this?

@sam-6174
Copy link

@sc0ttdav3y A "custom storage adapter" as you've suggested is interesting, but I'm wondering if there's another way to do this that completely avoids local storage. It could work something like this:

  1. Keep all tokens on the heap, e.g. store everything in a local JavaScript object, similar to how is shown in https://docs.amplify.aws/gen2/build-a-backend/auth/manage-user-session#custom-storage
    1. As is, the above implementation avoids security issues noted in this thread but would require user to login on every refresh or new tab/window.
  2. Modify the above by implementing:
    1. A /refresh endpoint on your domain that issues an http-only secure cookie, similar to what is described by @ravenscar here
    2. The logic within the custom storage:
      1. Upon initial page load, attempt to hydrate token storage contents via the /refresh endpoint. (This will work if we have a still-valid cookie from a prior page load, and we can reinstantiate storage contents without user interaction.)
      2. If above is unsuccessful (because we lack still-valid cookie) then render Amplify's Authentication component, let normal login flow happen (user clicks buttons to authenticate), and then use token storage contents to get our cookie from /refresh so that future page loads will "just work"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auth Related to Auth components/category Cognito Related to cognito issues needs-discussion Used for internal discussions Security Issues related to security concerns Service Team Issues asked to the Service Team
Projects
None yet
Development

No branches or pull requests