Forum OpenACS Development: Possible Vulnerablity in basic-info-update-2.tcl

1: Possible Vulnerablity in basic-info-update-2.tcl

Posted by Barry Books on 06/03/03 11:57 PM

I was looking at basic-info-update-2.tcl in acs 4.2 and it appears if you know a valid user_id you can change that users info. User_id is an input to the page and there is no check to see that it is either the logged in user or an admin.

I looked and the CVS head and I think the problem is mostly fixed by

ad_require_permission $user_id "write"

but it seems like there should also be a check to make sure that the user_id is in fact a user_id and not some other kind of id that user might have write access to.

Would it be usefull to have a page contract filter that could do this? It might also be nice to validate attributes. Perhaps

ad_page_contract {
} {
   {user_id:object.user ""}
   last_name:attribute.person.last_name
}

2: Re: Possible Vulnerablity in basic-info-update-2.tcl (response to 1)

Posted by Don Baccus on 06/04/03 04:06 AM

The perm check should be there, certainly. An additional and fast way to add more security is to sign and validate the user_id, which is supported directly in ad_page_contract.

Your ad_page_contract idea's interesting but I think in this particular case the two steps described above would be sufficient and don't require an extra trip to the database.

3: Re: Possible Vulnerablity in basic-info-update-2.tcl (response to 1)

Posted by russ m on 06/04/03 04:30 AM

Barry -

I don't think this is a problem. The ad_require_permission *is* a check that the account doing the update has the right to make changes to the object specified by user_id (and if your system permission hierarchy isn't reflecting reality you've probably got bigger problems).

Although the user_id parameter could in theory be the object_id of something other than a user, the updates that are actually carried out (in the db_transaction block at the end of the file) are restricted to tables and attributes that are appropriate for "user" entities.

The only way I can see that this page could be used outside it's designed purpose would be to change the recorded email address of a group that the person doing the update has write but not admin access to (I'm assuming that changing the email address of a group normally requires admin access).

4: Re: Possible Vulnerablity in basic-info-update-2.tcl (response to 1)

Posted by Barry Books on 06/04/03 01:35 PM

Signing the variable would be a good idea and would fix hacking attempts. Having a way to validate by object type would find programing errors. Perhaps not that important and would require a database hit. It could also check check that it's a number. Validating attributes seems more useful and would not require a database hit (at least for every one).

I found this problem by running a security scan against the site. It does things like put letters into the user_id and increment it and decrement it. It reports possible problems because this does not cause an error (other than a request error). The two classes of problems it finds are object_ids and parameter values are not properly validated. Most of the problems result in a request error. Most are not real problems but relying on variables really being the right object_id and the parameter really being the right size and type may not be the right defense against hacking either. Oracle will report an error but they might be used for other things.

If you are running 4.2 I suggest signing the user_id. The current version appears to let you change the email and url of any party (including groups) you have write access to. You can also add a bio to any object you have write access to. While these may not be fatal they are certainly not a feature either.

5: Re: Possible Vulnerablity in basic-info-update-2.tcl (response to 1)

Posted by Don Baccus on 06/05/03 06:27 PM

Well the current scheme assumes you know the type of what you're passing, and that you'll say "user_id:integer,notnull" when you write your page contract. If you find pages not doing that, submit patches to your heart's content.

Using db attributes would add some additional security, relieving the programmer of some the burden, but the db hit would be too expense IMO ... I think ad_page_contract, properly used, is a reasonable compromise.