Skip to content
Snippets Groups Projects
  1. Mar 06, 2018
  2. Feb 19, 2018
    • Gregory Szorc's avatar
      hgweb: always perform permissions checks on protocol commands (BC) (SEC) · 2ecb0fc5
      Gregory Szorc authored
      Previously, the HTTP request handling code would only perform
      permissions checking on a wire protocol command if that wire protocol
      command defined its permissions / operation type. This meant that
      commands (possibly provided by extensions) not defining their
      operation type would bypass permissions check. This could lead
      to exfiltration of data from servers and mutating repositories that
      were supposed to be read-only.
      
      This security issue has been present since the permissions table
      was introduced by d3147b4e3e8a in 2008.
      
      This commit changes the behavior of the HTTP server to always
      perform permissions checking for protocol requests. If an
      explicit permission for a wire protocol command is not defined,
      the server assumes the command can be used for writing and
      governs access accordingly.
      
      .. bc::
      
         Wire protocol commands not defining their operation type in
         ``wireproto.PERMISSIONS`` are now assumed to be used for
         "push" operations and access control to run those commands
         is now enforced accordingly.
      2ecb0fc5
  3. Feb 21, 2018
    • Gregory Szorc's avatar
      wireproto: check permissions when executing "batch" command (BC) (SEC) · ff4bc0ab
      Gregory Szorc authored
      For as long as the "batch" command has existed (introduced by
      bd88561afb4b and first released as part of Mercurial 1.9), that command
      (like most wire commands introduced after 2008) lacked an entry in
      the hgweb permissions table. And since we don't verify permissions if
      an entry is missing from the permissions table, this meant that
      executing a command via "batch" would bypass all permissions
      checks.
      
      The security implications are significant: a Mercurial HTTP server
      would allow writes via "batch" wire protocol commands as long as
      the HTTP request were processed by Mercurial and the process running
      the Mercurial HTTP server had write access to the repository. The
      Mercurial defaults of servers being read-only and the various web.*
      config options to define access control were bypassed.
      
      In addition, "batch" could be used to exfiltrate data from servers
      that were configured to not allow read access.
      
      Both forms of permissions bypass could be mitigated to some extent
      by using HTTP authentication. This would prevent HTTP requests from
      hitting Mercurial's server logic. However, any authenticated request
      would still be able to bypass permissions checks via "batch" commands.
      
      The easiest exploit was to send "pushkey" commands via "batch" and
      modify the state of bookmarks, phases, and obsolescence markers.
      However, I suspect a well-crafted HTTP request could trick the server
      into running the "unbundle" wire protocol command, effectively
      performing a full `hg push` to create new changesets on the remote.
      
      This commit plugs this gaping security hole by having the "batch"
      command perform permissions checking on each sub-command that is
      being batched. We do this by threading a permissions checking
      callable all the way to the protocol handler. The threading is a
      bit hacky from a code perspective. But it preserves API compatibility,
      which is the proper thing to do on the stable branch.
      
      One of the subtle things we do is assume that a command with an
      undefined permission is a "push" command. This is the safest thing to
      do from a security perspective: we don't want to take chances that
      a command could perform a write even though the server is configured
      to not allow writes.
      
      As the test changes demonstrate, it is no longer possible to bypass
      permissions via the "batch" wire protocol command.
      
      .. bc::
      
         The "batch" wire protocol command now enforces permissions of
         each invoked sub-command. Wire protocol commands must define
         their operation type or the "batch" command will assume they
         can write data and will prevent their execution on HTTP servers
         unless the HTTP request method is POST, the server is configured
         to allow pushes, and the (possibly authenticated) HTTP user is
         authorized to perform a push.
      ff4bc0ab
    • Gregory Szorc's avatar
      wireproto: declare operation type for most commands (BC) (SEC) · e3c228b4
      Gregory Szorc authored
      The permissions model of hgweb relies on a dictionary to declare
      the operation associated with each command - either "pull" or
      "push." This dictionary was established by d3147b4e3e8a in 2008.
      Unfortunately, we neglected to update this dictionary as new
      wire protocol commands were introduced.
      
      This commit defines the operations of most wire protocol commands
      in the permissions dictionary. The "batch" command is omitted because
      it is special and requires a more complex solution.
      
      Since permissions checking is skipped unless a command has an entry in
      this dictionary (this security issue will be addressed in a subsequent
      commit), the practical effect of this change is that various wire
      protocol commands now HTTP 401 if web.deny_read or web.allow-pull,
      etc are set to deny access. This is reflected by test changes. Note
      how various `hg pull` and `hg push` operations now fail before
      discovery. (They fail during the initial "capabilities" request.)
      
      This change fixes a security issue where built-in wire protocol
      commands would return repository data even if the web config were
      configured to deny access to that data.
      
      I'm on the fence as to whether we should HTTP 401 the capabilities
      request. On one hand, it can expose repository metadata and can tell
      callers things like what version of Mercurial the server is running.
      On the other hand, a client may need to know the capabilities in order
      to authenticate in a follow-up request. It appears that Mercurial
      clients handle the HTTP 401 on *any* protocol request, so we should
      be OK sending a 401 for "capabilities." But if this causes problems,
      it should be possible to allow "capabilities" to always work.
      
      .. bc::
      
         Various read-only wire protocol commands now return HTTP 401
         Unauthorized if the hgweb configuration denies read/pull access to
         the repository.
      
         Previously, various wire protocol commands would still work and
         return data if read access was disabled.
      e3c228b4
    • Gregory Szorc's avatar
      wireproto: move command permissions dict out of hgweb_mod · 742ce6fb
      Gregory Szorc authored
      The operation type associated with wire protocol commands is supposed
      to be defined in a dictionary so it can be used for permissions
      checking.
      
      Since this metadata is closely associated with wire protocol commands
      themselves, it makes sense to define it in the same module where
      wire protocol commands are defined.
      
      This commit moves hgweb_mod.perms to wireproto.PERMISSIONS and
      updates most references in the code to use the new home. The old
      symbol remains an alias for the new symbol. Tests pass with the
      code pointing at the old symbol. So this should be API compatible
      for extensions.
      
      As part of the code move, we split up the assignment to the dict
      so it is next to the @wireprotocommand. This reinforces that a
      @wireprotocommand should have an entry in this dict.
      
      In the future, we'll want to declare permissions as part of the
      @wireprotocommand decorator. But this isn't appropriate for the
      stable branch.
      742ce6fb
    • Gregory Szorc's avatar
      tests: comprehensively test HTTP server permissions checking · bbd4027b
      Gregory Szorc authored
      We didn't have test coverage for numerous web.* config options. We
      add that test coverage.
      
      Included in the tests are tests for custom commands. We have commands
      that are supposedly read-only and perform writes and a variation of
      each that does and does not define its operation type in
      hgweb_mod.perms.
      
      The tests reveal a handful of security bugs related to permissions
      checking. Subsequent commits will address these security bugs.
      bbd4027b
  4. Feb 18, 2018
    • Gregory Szorc's avatar
      tests: extract HTTP permissions tests to own test file · 2c647da8
      Gregory Szorc authored
      We're about to implement a lot more coverage of the permissions
      mechanism. In preparation for that, establish a new test file
      to hold permissions checks.
      
      As part of this, we inline the important parts of the "req" helper
      function.
      2c647da8
  5. Mar 06, 2018
  6. Feb 13, 2018
  7. Feb 07, 2018
    • Jun Wu's avatar
      changegroup: do not delta lfs revisions · d031609b
      Jun Wu authored
      There is no way to distinguish whether a delta base is LFS or non-LFS.
      
      If the delta is against LFS rawtext, and the client trying to apply it has
      the base revision stored as fulltext, the delta (aka. bundle) will fail to
      apply.
      
      This patch forbids using delta for LFS revisions in changegroup so bad
      deltas won't be transmitted.
      
      Note: this does not solve the problem entirely. It solves LFS delta applying
      to non-LFS base. But the other direction: non-LFS delta applying to LFS base
      is not solved yet.
      
      Differential Revision: https://phab.mercurial-scm.org/D2067
      d031609b
    • Jun Wu's avatar
      lfs: add a test showing bundle application could be broken · 4e41b596
      Jun Wu authored
      When a bundle containing LFS delta uses non-LFS delta-base, or vice-versa,
      the bundle will fail to apply.
      
      Differential Revision: https://phab.mercurial-scm.org/D2066
      4e41b596
  8. Mar 04, 2018
  9. Mar 01, 2018
  10. Feb 28, 2018
    • Matt Harbison's avatar
      test-subrepo: demonstrate problems with subrepo sharing and absolute paths · 0c14b3f2
      Matt Harbison authored
      This affects remote paths in .hgsub, as well as clone pooling from a remote
      source.
      
      For reasons unknown, there are stability issues with the relative-path.t tests.
      If run as a single test, it is stable.  If run with --loop, or with -jX for X>1,
      the hash of the parent repo changes.  I'm seeing this on both Windows and Fedora
      26.  I added an `hg log --debug`, and the manifest hash changes, but I have no
      idea why.
      0c14b3f2
  11. Feb 21, 2018
    • Yuya Nishihara's avatar
      annotate: do not poorly split lines at CR (issue5798) · 0a7c59a4
      Yuya Nishihara authored
      mdiff and lines(text) take only LF as a line separator, but str.splitlines()
      breaks our assumption. Use mdiff.splitnewlines() consistently.
      
      It's hard to read \r in tests, so \r is replaced with [CR]. I had to wrap
      sed by a shell function to silence check-code warning.
      0a7c59a4
  12. Feb 24, 2018
    • Gregory Szorc's avatar
      setup: only allow Python 3 from a source checkout (issue5804) · fb39f6a8
      Gregory Szorc authored
      People are running `pip install Mercurial` with Python 3 and that
      is working because not everything performs a Python version
      compatibility check.
      
      Modern versions of pip do recognize the "python_requires" keyword
      (https://packaging.python.org/tutorials/distributing-packages/#python-requires)
      which we set if using setuptools. But this isn't set nor recognized
      everywhere.
      
      To prevent people from accidentally installing Mercurial with Python
      3 until Python 3 is officially supported, have setup.py fail when
      run with Python 3. But don't fail if we're running from a source
      checkout, as we don't want to anger Mercurial developers hacking
      on Python 3 nor Mercurial's test automation running from source
      checkouts. People running setup.py from source checkouts could still
      fall through a Python 3 crack. But at least the
      `pip install Mercurial` attempt will get nipped in the bud.
      fb39f6a8
  13. Feb 21, 2018
  14. Feb 22, 2018
  15. Feb 19, 2018
    • Gregory Szorc's avatar
      tests: expand test coverage for updating phases · c19e66da
      Gregory Szorc authored
      Consolidating the tests demonstrated that there are behavior
      differences when pushing phases between bundle1 and bundle2.
      
      A reason for this is the behavior of legacy pushes where the client
      queries the state of phases and then conditionally updates phases
      after an "unbundle" is processed. This behavior is expected.
      
      The tests were incomplete because they only tested the case of a
      publishing repo. In this commit, we add a variant for a non-publishing
      repo. We still see some differences between the legacy and bundle2
      exchanges. But they are less pronounced.
      
      The behavior of not firing a pushkey hook when phases are updated as
      part of changegroup application feels weird to me. I'm not sure if
      this is a feature or a bug. By the time the "pushkey" or "phases"
      bundle2 part is applied, the phases have already been moved on
      a publishing repository. We fire the "pushkey" hook regardless,
      even though it would be a no-op. This is the part that feels the
      most buggy.
      c19e66da
  16. Feb 18, 2018
    • Gregory Szorc's avatar
      tests: consolidate test-push-http.t and test-push-http-bundle1.t · 47728063
      Gregory Szorc authored
      These tests were initially copies of each other. Now that we have
      #testcases support in .t tests, we can consolidate them.
      
      The changes to test-push-http.t reflect the differences between that
      file and test-push-http-bundle1.t.
      
      The variances in phases push behavior are the biggest differences.
      The test will be updated in a subsequent commit to make the differences
      more clear and to expand test coverage. For now, let's just port
      the differences verbatim to get the tests consolidated.
      47728063
    • Gregory Szorc's avatar
      tests: port value-less unbundle capability test to test-push-http.t · fefd57bd
      Gregory Szorc authored
      This test is present in test-push-http-bundle1.t. Let's add it to
      test-push-http.t to further unify the tests.
      fefd57bd
    • Gregory Szorc's avatar
      tests: add phase testing to test-push-http-bundle1.t · e978e0c1
      Gregory Szorc authored
      test-push-http.t and test-push-http-bundle1.t were initially copies.
      Now that we have support for inline test variants, we can combine them.
      
      One of the variances between the tests is testing of phase moving.
      We add the missing code to test-push-http-bundle1.t.
      e978e0c1
  17. Feb 16, 2018
    • Jun Wu's avatar
      date: fix parsing months · 48783333
      Jun Wu authored
      Thanks nemo for discovering this on #mercurial IRC channel.
      
      Test Plan:
      Add a test. It fails before this patch:
      
      ```
      +  hg: parse error: invalid date: 'Feb 2018'
      +  hg: parse error: invalid date: 'Apr 2018'
      +  hg: parse error: invalid date: 'Jun 2018'
      +  hg: parse error: invalid date: 'Sep 2018'
      +  hg: parse error: invalid date: 'Nov 2018'
      ```
      
      Differential Revision: https://phab.mercurial-scm.org/D2289
      48783333
  18. Feb 13, 2018
  19. Feb 07, 2018
  20. Feb 01, 2018
  21. Jan 31, 2018
  22. Jan 28, 2018
    • Matt Harbison's avatar
      revset: evaluate filesets against each revision for 'file()' (issue5778) · f6ca1e11
      Matt Harbison authored
      After f2aeff8a87b6, the fileset was evaluated to a set of files against the
      working directory, and then those files were applied against each revision.  The
      result was nonsense.  For example, `hg log -r 'file("set:exec()")'` on the
      Mercurial repo listed revision 0 because it has the `hg` script, which is
      currently +x.  But that bit wasn't applied until revision 280 (which
      'contains()' properly indicates).
      
      This technique was borrowed from checkstatus(), which services adds(),
      modifies(), and removes(), so it seems safe enough.  The 'r:' case is explicitly
      assigned to wdirrev, freeing up rev=None to mean "re-evaluate at each revision".
      The distinction is important to avoid behavior changes with `hg log set:...`
      (test-largefiles-misc.t and test-fileset-generated.t drop current log output
      without this).  I'm not sure what the right behavior for that is (1fd352aa08fc
      explicitly enabled this behavior for graphlog), but the day before the release
      isn't the time to experiment.
      f6ca1e11
  23. Feb 01, 2018
  24. Jan 31, 2018
Loading