[SECURITY] Users with read-only permissions may alter bookmarks, phases and obsolescence markers in a Hg repository

Issue #970 resolved
Gábor Stefanik
created an issue

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.

Comments (20)

  1. Sebastian Sdorra repo owner

    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.

  2. Sebastian Sdorra repo owner

    I've added some ngrep dumps of mercurial requests to the repository, in order to understand the wire protocol better and i've started to improve the isWriteRequest tests with more realistic data:

  3. Sebastian Sdorra repo owner

    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\=pushkey
    

    The same command with "-X POST" works.

    So i don't think that it is possible to modify a repository without write permissions.

  4. Sebastian Sdorra repo owner

    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
    
  5. Sebastian Sdorra repo owner

    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:

    • handle every request as write, expect for a set of whitelisted commands
    • batch requests are only treated as read, if all their commands are in the white list
    • post request are always treated as write, regardless of the command

    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?

  6. Gábor Stefanik reporter

    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).

  7. Gábor Stefanik reporter

    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).

  8. Sebastian Sdorra repo owner

    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:

    No need to hack configuration files, SCM-Manager is completely configureable from its Web-Interface
    

    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.

  9. Gábor Stefanik reporter

    If I currently have the Ubuntu deb package installed on the test system using apt, how do I install this version keeping the existing settings? Is it OK to just replace scm-webapp.war?

  10. Log in to comment