sandbox::TouchMemory overwrites parameters
Reported by
lordprot...@gmail.com,
May 4 2018
|
|||
Issue descriptionI’ve been playing with your sandbox recently and noticed that my dummy sandboxed application is crashing when I have ProcessStrictHandleCheckPolicy turned on. After some digging I found out that the INVALID_HANDLE exception is thrown from CopyFile, which doesn’t take any handles, just paths. So at first I thought it’s bug in API. After some debugging I found the spot where the API calls NtCreateFile which fails STATUS_ACCESS_DENIED (kudos to sandbox working properly) but due to a design of the API the return code is checked but the API doesn’t immediately quit as you would expect but also checks whether the returned handle doesn’t satisfy the following formula ((HANDLE + 1) & 0xFFFFFFFFFFFFFFFE). If it does, it assumes that the handle is valid and starts calling other API with it. This of course works in unsandboxed code just fine because the HANDLE is set to INVALID_HANDLE_VALUE and that doesn’t satisfy the formula. But in sandboxed code sandbox::TouchMemory is called on this value before it’s send to broker, this function overwrites the original INVALID_HANDLE_VALUE (0xffffffff) to 0x00ffff00 which as you might see it’s not ok. Shouldn’t sandbox::TouchMemory restore the original bytes after verifying that the memory supplied by user is writable? The issue can be replicated on Windows 10 by using ProcessStrictHandleCheckPolicy policy and trying to copy a file where target has only read access.
,
May 7 2018
hmm interesting, thanks for this report. calling ValidParameter with WRITE doesn't guarantee that the parameter is left untouched since it's supposed to be called before any data is written into it - see https://cs.chromium.org/chromium/src/sandbox/win/src/sandbox_nt_util.h?l=103 but it sounds like we should be re-initializing this, or at least setting it correctly.
,
May 7 2018
Thanks for looking into it. Do you want me to create a pull request or will you look at it?
,
May 9 2018
This sounds like a bug in the implementation of CopyFile, it shouldn't make any assumptions about the value of the handle being returned if the system call fails. Just because it's set it to some expected value doesn't mean that it'll return any specific value afterwards. That said with the sandbox we have to try and work around this. It would seem to make sense to read the original value from the destination and write it back. This would need to be done carefully as I could easily envisage the compiler optimizing out something simple like *p = *p. I'd expect if we made the pointers volatile it should stop the compiler from optimizing it out, but we'd want to make sure that it's what we think it's compiling is what is actually happening.
,
May 15 2018
hi lordprotector, if you want to put up a CL that would be great, I'm happy to review. I don't think we are likely to get to this issue in a hurry otherwise since we currently expect CopyFile to fail from within the sandbox anyway (and no sandbox component of Chromium should use that API). I'd probably start with an integration tests (in sbox_integration_test) to verify the issue first.
,
May 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9466f2db2d1d17d12ba472a57a3382bb20dc196e commit 9466f2db2d1d17d12ba472a57a3382bb20dc196e Author: Richard Baranyi <lordprotector@gmail.com> Date: Wed May 16 20:58:42 2018 sandbox::TouchMemory now doesn't destroy input buffer Windows 10 RS4 (maybe older as well) x64 implementation of CopyFile expects that the HANDLE passed to NtCreateFile is not modified upon error. Old implementation of TouchMemory 'destroys' the buffer before passing it to the broker. This can be fixed by first reading the original value and then restoring back at the original place. This has to be done carefully so optimizer won't remove the code. R=wfh@chromium.org Bug: 839775 Change-Id: Iff80f3a5f145b85da2f0ba500e74387fce4e0e4b Reviewed-on: https://chromium-review.googlesource.com/1061460 Reviewed-by: Will Harris <wfh@chromium.org> Commit-Queue: Will Harris <wfh@chromium.org> Cr-Commit-Position: refs/heads/master@{#559279} [modify] https://crrev.com/9466f2db2d1d17d12ba472a57a3382bb20dc196e/AUTHORS [modify] https://crrev.com/9466f2db2d1d17d12ba472a57a3382bb20dc196e/sandbox/win/src/file_policy_test.cc [modify] https://crrev.com/9466f2db2d1d17d12ba472a57a3382bb20dc196e/sandbox/win/src/sandbox_nt_util.cc [modify] https://crrev.com/9466f2db2d1d17d12ba472a57a3382bb20dc196e/sandbox/win/src/sandbox_nt_util.h
,
May 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/245f22db350e927617ce0f7374e5fee289864854 commit 245f22db350e927617ce0f7374e5fee289864854 Author: Richard Baranyi <lordprotector@gmail.com> Date: Thu May 17 17:55:15 2018 Add unit test for sandbox::ValidParameter The change introduced in commit 9466f2db2d1d17d12ba472a57a3382bb20dc196e can be potentialy optimized out by compiler because it might think nothing is changed when you read and store the original value at the same place. This would be undesirable so it's better to have a unit test for this case. R=wfh@chromium.org Bug: 839775 Change-Id: Id1f3ab978698a3d552734e01b77469643d287c09 Reviewed-on: https://chromium-review.googlesource.com/1063610 Commit-Queue: Will Harris <wfh@chromium.org> Reviewed-by: Will Harris <wfh@chromium.org> Cr-Commit-Position: refs/heads/master@{#559599} [modify] https://crrev.com/245f22db350e927617ce0f7374e5fee289864854/sandbox/win/src/sandbox_nt_util_unittest.cc
,
Jun 17 2018
I'd say this can be closed now.
,
Jun 17 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by dtapu...@chromium.org
, May 4 2018