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. -- Mailing list: https://launchpad.net/~mugle-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~mugle-dev More help : https://help.launchpad.net/ListHelp
