Security Flaw with localStorage

[sarj21]

There is a major security issue with how the user data is stored and accessed.

First off, in Session.js when a new Session object is created the user is stored in local storage

    this.create = function(token, user){
      ...
      $window.localStorage.currentUser = JSON.stringify(user);
      ...
    };

This seems fine as they are themselves, however, in routes.js

     if (requireAdmin && !Session.getUser().admin) {
        return transition.router.stateService.target("app.dashboard");
      }

      if (requireVerified && !Session.getUser().verified) {
        return transition.router.stateService.target("app.dashboard");
      }

      if (requireAdmitted && !Session.getUser().status.admitted) {
        return transition.router.stateService.target("app.dashboard");
      }

We get from Session.getUser which just fetches the user from localStorage. So, if a user just opens the inspector and changes the localStorage object:

> localStorage.getItem("currentUser")
< // the currentUser object
> localStorage.setItem("currentUser", {...,'admin': true,...}) // copied from above

they can gain access to the admin tab

We actually had an attendee of our hackathon note this to us and we are mostly admins already so we didn't recreate exactly before changing this. Either way, I made a change that makes Session.getUser use the existing users/:id route in the api and I still store the user id in local storage, as that seems relatively harmless

[YasserDRIF]

Yeah that's true, but ...
First to try this easily you can add a break-point in the build app.js after it was loaded

this.getUser = function(){
  return JSON.parse($window.localStorage.currentUser);
};

and then in the inspector you run these commands

t=JSON.parse($window.localStorage.currentUser)
t.admin=true
$window.localStorage.currentUser=JSON.stringify(t)

This will show you the admin view but of course no info will show in the screen as all of it is stored in the database and the if in the route you added 'isAdmin' function

router.get("/users/stats", isAdmin, function(req, res) {
UserController.getStats(defaultResponse(req, res));
});

and in 'isAdmin' the verification is being done using the user's token

function isAdmin(req, res, next) {
var token = getToken(req);

UserController.getByToken(token, function(err, user) {      // from DB
  if (err) {
    return res.status(500).send(err);
  }

  if (user && user.admin) {   // verification
    req.user = user;
    return next();
  }

  return res.status(401).send({
    message: "Get outta here, punk!"
  });
});
}

So he won't get anything that you marked as require Admin in the API
I guess this should be changed so that the user won't even be able to see the admin views but it's not something major

Can you please share the modifications you did to your code to fix this?

[sarj21]

Yeah I think I even mentioned to the team it wasn't that damaging as I suspected they really couldn't get any info so you are correct, the admin views are back end validated at some point.

Here's the commits:

vthacks-org@060a462
vthacks-org@5d23944

I made a quick change to the Session.js to remove any reference to current user:

 this.create = function(token, user){
        $window.localStorage.jwt = token;
        $window.localStorage.userId = user._id;
        $rootScope.currentUser = user;
      };

      this.destroy = function(onComplete){
        delete $window.localStorage.jwt;
        delete $window.localStorage.userId;
        $rootScope.currentUser = null;
        if (onComplete){
          onComplete();
        }
      };

...

      this.getUser = function(){
        var http = $injector.get('$http');

        return http.get('/api/users/' + $window.localStorage.userId);
      };

and then on the routes.js checked if they had a token and if they did then checked the user against the api:

        if (requireLogin && !Session.getToken()) {
          event.preventDefault();
          $state.go('login', null, {
            location: 'replace',
          });
        } else {
          Session.getUser().then( (res) => {
            var user = res.data; 
    
            if (requireAdmin && !user.admin) {
              event.preventDefault();
              $state.go('app.dashboard');
            }
    
            if (requireVolunteerOrAdmin && !user.volunteer && !user.admin){
              event.preventDefault();
              $state.go('app.dashboard');
            }
    
            if (requireVerified && !user.verified){
              event.preventDefault();
              $state.go('app.dashboard');
            }
            
            if (requireAdmitted && !user.status.admitted) {
              event.preventDefault();
              $state.go('app.dashboard');
            }
  
          });
        }

but this is a kind of janky hot fix I put in place while I looked into it. It does seem to work, though. I bet there's a better fix that might be a little more far reaching. The session should probably be tracked with a cookie instead of via localStorage but that might take a bit more work, if we end up going that route I will happily change it and submit it.

[patins]

TLDR hiding an interface from users of a client-side app requires never delivering the interface, but deliving the admin interface probably isn't harmful

Currently, hiding the admin interface (just the UI, not admin facing content such as user info) from users is not a feature in Quill. All Angular templates are accessible by the client, regardless of admin status.

To get around the proposed fix, just add a breakpoint after the API call or intercept the request itself.

If hiding the admin templates/frontend logic is super important, I'd recommend building a separate angular app that does authentication before delivery. This is only necessary if the admin interface contains features you don't want users to be aware of.

[sarj21]

I think intercepting the request might be a good idea.

I know, just in general, storing volatile things in local storage is not usually recommended (e.g. user data). Like we have said, they can't actually harm anything or do any damage so there is no actual security flaw.

In general, they shouldn't be able to access admin though and I think my fix does a good job at just not allowing that the way this person tried it. You can close this issue if the existing behavior is fine and since its not dangerous and as you said, angular templates are all accessible by the client.