[SECURITY] Users with read-only permissions may alter bookmarks, phases and obsolescence markers in a Hg repository #970
Comments
|
Original comment by Sebastian Sdorra (Bitbucket: sdorra, GitHub: sdorra). Ok, i think you are right and we need an other way to decide between read and write requests. The only way i see at the moment is to decide by the commands of the wire protocol. I think i would treat every request as write request, expect the used command (in case of batch all commands) is white listed as read commands. But this looks not easy, because we have to check for the cmd query parameter and in the case of the batch command we have to check for cmds in the x-hgargs-? header. The second problem with this method seems to be the httppostargs (issue #944), because for this case we have to read the whole body of the request to get the x-hgargs-? parameters and this could be a huge performance problem. |
|
Original comment by Sebastian Sdorra (Bitbucket: sdorra, GitHub: sdorra). I've also tried to create a bookmark with a get request, but mercurial denied that with 405 Method Not Allowed. Here is what i've tested: curl -u "scmadmin:scmadmin" \
-X GET \
-H "Accept-Encoding: identity" \
-H "content-type: application/mercurial-0.1" \
-H "vary: X-HgArg-1" \
-H "x-hgarg-1: key=markthree&namespace=bookmarks&new=187ddf37e237c370514487a0bb1a226f11a780b3&old=" \
http://localhost:8080/scm/hg/hgtest\?cmd\=pushkeyThe same command with "-X POST" works. So i don't think that it is possible to modify a repository without write permissions. |
|
Original comment by Sebastian Sdorra (Bitbucket: sdorra, GitHub: sdorra). Ok, you are right. I was able to reproduce the it: curl -u "scmadmin:scmadmin" \
-X GET \
-H "Accept-Encoding: identity" \
-H "content-type: application/mercurial-0.1" \
-H "vary: X-HgArg-1" \
-H "x-hgarg-1: cmds=pushkey key=markthree,namespace=bookmarks,new=187ddf37e237c370514487a0bb1a226f11a780b3,old=" \
http://localhost:8080/scm/hg/hgtest\?cmd\=batch |
|
Original comment by Sebastian Sdorra (Bitbucket: sdorra, GitHub: sdorra). It looks like more a problem with the wire protocol itself (see release notes), however i will try to fix this, but as i mentioned at comment 44239862 it could be hard. I think we could implement the permission check as follows:
I know the last part will not work with the httppostargs based protocol, but i think we have to fix the security problem first and then we could try to find a secure and performant way for the httppostargs. What do you think? |
|
Original comment by Gábor Stefanik (Bitbucket: gstefanik, GitHub: gstefanik). Once we whitelist request types, it's a bad idea to check for POST. All that check will do is break httppostargs. Just verb-agnostically check the commands. No need to check all POST request bodies in the httppostargs case, only those that have an X-HgArgs-Post header, and even then, only the first X-HgArgs-Post bytes need to be checked (or rather, added to the X-HgArg-N headers). |
|
Original comment by Sebastian Sdorra (Bitbucket: sdorra, GitHub: sdorra). Could you please test the version below: The version contains the following patch: This version should prevent the privilege escalation, but the httppostargs is not yet supported. It is also possible that you have a look at the tests of HgPermissionFilterTest and WireProtocolTest. Did i miss something? I will now try to implement the httppostargs thing. |
|
Original comment by Gábor Stefanik (Bitbucket: gstefanik, GitHub: gstefanik). The first patch looks good, thanks. I would rather not expose an [experimental] config option through the web UI, as [experimental] is reserved for options that are bound to change. In fact, I am planning to propose a change to move the httppostargs option to [web], and change it from a boolean to an integer representing the minimum request size to send as POST (so that only large requests that need to be turned into POST are actually sent as POST). Requiring users to set it using hgrc is probably fine. What needs to be done is parsing the first X-HgArgs-Post bytes of the POST body as if it were an X-HgArg-N header - Hg will parse the first X-HgArgs-Post bytes of the body as arguments if that header is set, regardless of whether the httppostargs option is actually enabled, although no current legit client will ever set X-HgArgs-Post when accessing a server that doesn't have httppostargs enabled on the server side (but that's exactly what I am planning to do in Hg 4.6). |
|
Original comment by Sebastian Sdorra (Bitbucket: sdorra, GitHub: sdorra). I don't think that it is fine to edit the hgrc, because one of the principles of scm-manager is the easy usage. Here the quote from the start page of scm-manager.org: So i think we have to create a new version, when the changes of mercurial are released. I think we could support both options the experimental and the web one. However i've created a new test version, which supports the httppostargs. Could you please test the version below: This version includes the following patch: Note: The option must be enabled in the mercurial configuration at repository types. |
Original report by Gábor Stefanik (Bitbucket: gstefanik, GitHub: gstefanik).
The Hg wireproto command "pushkey" can be sent over a HTTP GET request, either alone, or packaged in a "batch" command. Because we use the HTTP method to tell if a Hg request is read or write, "pushkey" is wrongly treated as a read request, and allowed for any user with just a read permission (including unauthenticated users, if the repository is public and "Allow Anonymous Access" is enabled).
The "pushkey" command can be used to modify bookmarks, set obsolescence markers, or advance phase boundaries in the repository.
Even worse, it's possible that the "unbundle" command may be sent using a whitelisted request type as well (e.g. as a malformed GET or OPTIONS with a body), which would essentially enable an unauthorized push.
This issue was discovered during the workup of issue #944. It's essentially the far more dangerous reverse of #944 - instead of mistaking a read request for a write because it's packaged as a POST, we mistake a write for a read because it's sent as a GET.
The text was updated successfully, but these errors were encountered: