mugle-dev team mailing list archive

[Matt Giuca <786876@xxxxxxxxxxxxxxxxxx>]

If you look at UserServiceImpl, that is implemented correctly (I assume
this one was implemented first, and the others were copied). All of the
ServiceImpls are using logic that compares the current user's key to the
object's key, which is nonsense (completely different types), EXCEPT for
when comparing to user objects, where it makes sense.

That one has:

getByKey - compare curUser to user
getByName - compare curUser to user
getLoggedInUser - always private (OK, since it's always the current user)
getDevTeams - always private (OK, since the devteams belong to the current user)
getUserGameProfiles - always private (OK, since the profiles belong to the current user)
getUsers - always public (slightly wrong, since it doesn't give all possible details about the current user, but at least it's secure)
getUsersByRole - always public
checkPermissions - compare curUser to user

What I don't understand is why there is an abstraction,
checkPermissions, but it is only used sometimes, and other times, the
check is hard-coded. Is there a reason why checkPermissions is
inappropriate in some cases?

I also don't understand how the checks can specify "check read
permissions" vs "check write permissions", but perhaps that comes when
you are actually reading/writing the object.

Also, I take back what I said about the incorrectly-typed key
comparisons defaulting to private. They seem to default to public,
though when I run it I am usually getting private access anyway, for
some reason.

Full audit:

AchievementServiceImpl:
getByKey - always private (INSECURE)
getByName - always private (INSECURE)
getUserAchievements - always private (I'm not sure what this is doing; it doesn't seem to be getting achievements for any particular user)
checkPermissions - compare curUser to Achievement key (INCORRECT, always public)

DevTeamServiceImpl:
getByKey - always private (INSECURE)
getByName - always private (INSECURE)
create - compare curUser to DevTeam key (INCORRECT, always public)
getGames - always private (INSECURE)
getUsers - always private (INSECURE)
getDevTeams - always public
remove - compare curUser to DevTeam key (INCORRECT, always public)
checkPermissions - compare curUser to DevTeam key (INCORRECT, always public)

GameFileServiceImpl:
getByKey - always private (INSECURE)
getByPath - always private (INSECURE)
checkPermissions - compare curUser to GameFile key (INCORRECT, always public)

GameServiceImpl:
getByKey - always private (INSECURE)
getByName - always private (INSECURE)
getByGameToken - always private (INSECURE)
getAchievements - always private (INSECURE)
getGameVersions - always private (INSECURE)
getUserGameProfiles - always private (INSECURE)
checkPermissions - compare curUser to Game key (INCORRECT, always public)
getGames - always public
getActiveGames - always public
getMyGames - always public

GameVersionServiceImpl:
getByKey - always private (INSECURE)
getByName - always private (INSECURE)
getGameFiles - always private (INSECURE)
checkPermissions - compare curUser to GameVersion key (INCORRECT, always public)

KeyValuePairServiceImpl:
getByKey - always private (INSECURE; also does some checking but it doesn't look right -- why is it talking about writing when all we are doing is getting?)
getByPairKey - always private (INSECURE)
checkPermissions - compare curUser to KeyValuePair key (INCORRECT, always public)

PromotedGameServiceImpl:
getByKey - always private (INSECURE)
getByName - always private (INSECURE)
checkPermissions - compare curUser to PromotedGame key (INCORRECT, always public)

UserAchievementServiceImpl:
getByKey - always private (INSECURE)
getByUserGameProfile - always private (INSECURE)
checkPermissions - compare curUser to UserAchievement key (INCORRECT, always public)

UserGameProfileServiceImpl:
getByKey - always private (INSECURE)
getByUserAndGame - always private (INSECURE)
getKeyValuePairs - always private (INSECURE)
getUserAchievements - always private (INSECURE)
checkPermissions - compare curUser to UserGameProfile key (INCORRECT, always public)

In short, EVERY class (besides UserServiceImpl) needs to be fixed, in
pretty much the same way. As I asked above, there is a checkPermission
abstraction for this. Why aren't we using it for all checks? Is it
possible to rewrite all of the hand-written checks to use
checkPermission? If not, is there an alternative function we can write
that does the permission check for reads?

-- 
You received this bug notification because you are a member of MUGLE
Developers, which is a direct subscriber.
https://bugs.launchpad.net/bugs/786876

Title:
  Almost all data access is given private privileges

Status in Melbourne University Game-based Learning Environment:
  Triaged

Bug description:
  The data view system has been horribly abused. I haven't made a
  complete analysis of all the classes, but by the look of it, in most
  cases, objects are being presented with the private view whether the
  user owns it or not. Particularly the Game, GameVersion and GameFile
  exhibit this. The offending code is in the ServiceImpl classes, which
  assign the view type.

  At this point, most of our security is happening by accident, and can
  easily be subverted.

  It looks like most of the WRITES have buggy checks which end up
  resulting in "private" no matter what (eg, make it public if two keys
  of incompatible types are equal, but otherwise make it private). Most
  of the READS simply assign "private" without any checks at all. This
  code needs a complete audit.

  Note that you often don't notice these problems, because the role
  isn't high enough even for private access. For example, on GameData,
  most fields have public access for admin only, but private access for
  developers. That means guests can't see Games, but developers have
  full access to games - even ones they didn't create. This demonstrates
  the silliness of the developer role (bug #786842).

  This bug is responsible for bug #786685.