[CSM-25] Wallet secret key file mode #98

dniku · 2017-03-13T12:55:07Z

No description provided.

mention-bot · 2017-03-13T12:55:09Z

@Pastafarianist, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gromakovsky, @neongreen and @flyingleafe to be potential reviewers.

akegalj · 2017-03-13T13:11:01Z

src/Pos/Util/UserSecret.hs

+
+-- | Check whether a given file has mode 600.
+failIfModeNot600 :: (MonadIO m, MonadFail m) => FilePath -> m ()
+failIfModeNot600 path = do


I wouldn't fail. Why don't we just have:

isFileMode600 :: (MonadIO m, MonadFail m) => FilePath -> m Bool

?

akegalj · 2017-03-13T13:12:37Z

src/Pos/Util/UserSecret.hs

+    liftIO $
+#ifdef POSIX
+        if exists
+        then failIfModeNot600 path


if file mode isn't 600 we should:

Create a temporary file with the right permissions (under /tmp or something, I hope there is a library or a library function for that; I have no idea how to do this on Mac OS, for example)

Write the key into the file

Force-move the new file over the old one (like mv -f)

why are we failing at all?

(/tmp works on OS X)

Writing to a temporary file and then moving also makes this atomic (sometimes). Not sure whether that matters here, but it could make sense to always do that.

@startling do you think it is ok to fail? This is our concern. For example user wants to run cardano-node with a key which is on disk, but it has wrong permissions. We can:

fail and notify user that he should manually change permissions

using steps described above ([CSM-25] Wallet secret key file mode #98 (comment)) just silently fix permissions

SSH fails in this situation. Should we do the same?

is this the only place a key would be stored?

@startling I think so, but I'm not sure. We should ask someone who participated in designing this part. @flyingleafe?

UPD: @flyingleafe says that the *.key files in the cardano-sl directory are the only place where private keys are stored, not counting RAM during execution.

I would tend towards exiting and having the user fix it, since this should never happen in normal operation?

Yes. In normal operation we will create keys with the correct permissions. Ok, we will leave this as it is

@Pastafarianist ⏫

this is quite racy, for something that could be done as one operation; you can create the file (O_CREAT) and fail if it already exist (O_EXCL), and the mode are automatically set to be the right one with the 3rd parameters of open.

It's probably possible with unix already, but using the handle branch of foundation, you can do that with:

import Foundation.System.Bindings.Posix withFilepath filepath $ \fp -> do fd <- sysPosixOpen fp (sysPosix_O_WRONLY .|. sysPosix_O_CREAT .|. sysPosix_O_EXCL .|. sysPosix_O_NONBLOCK) (CMode 0o600) ...

@vincenthz Apparently, unix doesn't support O_CREAT. There is an open bug about that and a pull request which claims to fix it. It has not been merged in almost a year though. Seems like the library's authors don't have a lot of interest in it. We could fork it.

startling · 2017-03-13T20:38:43Z

src/Pos/Util/UserSecret.hs

@@ -109,6 +158,7 @@ peekUserSecret path = takeReadLock path $ do
 -- 'writeUserSecretRelease'.
 takeUserSecret :: (MonadIO m) => FilePath -> m UserSecret
 takeUserSecret path = liftIO $ do
+    initializeUserSecret path


This can race under certain circumstances.

this code creates the file with mode 600

Attacker (assuming there's write access to the directory) swaps it out with one that doesn't have the same permissions.

this code finds it from the path and then writes to it, exposing secrets

I think this is marginal, since probably an attacker can't write to this directory. But it's much better to use the same file handle throughout.

@startling I don't see where this:

Attacker (assuming there's write access to the directory) swaps it out with one that doesn't have the same permissions.

this code finds it from the path and then writes to it, exposing secrets

can happen. Can you point to the code?

@startling could you elaborate how this is a problem? writeRaw always ensures that the key is written with the correct permissions, so the secret should never be exposed.

If it's a shared directory (like /tmp without per-user tmp), then you defintely need to keep your fd after creation (specially relevant for tmp file). otherwise for a user directory, that is presumably not readable/writable by anyone else that probably doesn't matter much

@vincenthz AFAIU in the current implementation it's a user directory (for me, the *.key files are created in the root of the cloned cardano-sl repo).

I see, I think I misunderstood.

vincenthz · 2017-03-14T19:49:23Z

src/Pos/Util/UserSecret.hs

+writeRaw u = do
+    let path = u ^. usPath
+    -- On POSIX platforms, openTempFile guarantees that the file
+    -- will be created with mode 600.


you probably mean openBinaryTempFile, and openTempFile is actually using 0o666 as default mask + whatever the process umask is.

@vincenthz you're right about openBinaryTempFile, I'll fix that. Also, please see the docs for openTempFile:

The file is created with permissions such that only the current user can read/write it.

neongreen · 2017-03-19T20:53:48Z

src/Pos/Util/UserSecret.hs

 {-# LANGUAGE TemplateHaskell #-}

 -- | Secret key file storage and management functions based on file
 -- locking.

+#if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)


It turned out that just #ifndef mingw32_HOST_OS is enough: https://ghc.haskell.org/trac/ghc/ticket/9945#comment:15

neongreen · 2017-03-19T20:54:33Z

src/Pos/Util/UserSecret.hs

-        "UserSecret { _usKeys = " ++ show _usKeys ++
-        ", _usVss = " ++ show _usVss ++
-        ", _usPath = " ++ show _usPath
+        "UserSecret { _usKeys = " <> show _usKeys <>


We usually use Formatting for this (grep for sformat)

neongreen · 2017-03-19T20:56:05Z

src/Pos/Util/UserSecret.hs

@@ -81,34 +99,70 @@ instance Bi UserSecret where
        keys <- get
        return $ def & usVss .~ vss & usPrimKey .~ pkey & usKeys .~ keys

+#ifdef POSIX
+-- | Constant that defines file mode 600.


It'd be nice if there was a comment saying what mode 600 corresponds to (afaik, “only owner can read and write”)

neongreen · 2017-03-19T20:59:36Z

src/Pos/Util/UserSecret.hs

+        throwM $ Internal $
+            "Key file access mode is incorrect. Set it to 600 and try again." <>
+            " Key file path: " <> show path <>
+            " Current mode: " <> toText (showIntAtBase 8 intToDigit accessMode "")


Such things can be done with Formatting too, using e.g. the oct formatter

neongreen · 2017-03-19T21:20:48Z

src/Pos/Util/UserSecret.hs

+    -- will be created with mode 600.
+    bracket
+        (openBinaryTempFile (takeDirectory path) (takeFileName path))
+        (\(tempPath, tempHandle) -> do


So you want the file to be created even if BSL.hPut tempHandle $ encode u fails?

Uhh... I've fixed this to a more stable version, but the code has become significantly uglier, and I would welcome any suggestions on how to improve it.

neongreen

I can't say much about the actual content of this PR – it looks okay to me but keep in mind that I'm not a systems guy. I've requested some stylistic changes, though.

vincenthz

considering that it's use in a user directory, it seems fine.

domenkozar · 2017-03-28T08:09:54Z

src/Pos/Util/UserSecret.hs

 {-# LANGUAGE TemplateHaskell #-}

 -- | Secret key file storage and management functions based on file
 -- locking.

+#if !defined(mingw32_HOST_OS)
+#define POSIX
+#endif


Such redefinitions only introduce confusion in the code, are they really necessary?

domenkozar · 2017-03-28T08:18:00Z

src/Pos/Util/UserSecret.hs

+#ifdef POSIX
+    failIfModeNot600 path
+#endif
+    content <- either (throwM . Internal . toText) pure . decodeFull =<< BSL.readFile path


Internal is not defined according to appveyor

domenkozar · 2017-03-28T10:59:42Z

@akegalj we are using -Werror now:

C:\projects\cardano-sl\srcPosUtilUserSecret.hs:31:1: warning: [-Wunused-imports]
        The import of `oct, sformat' from module `Formatting' is redundant
    
    <no location info>: error:

akegalj · 2017-03-28T11:59:21Z

@domenkozar right. I will remove warnings now

…t-key-permissions

[CSE-98] Clone merge

* Add preliminar tests trying to reproduce the bug * Successfully replicate the bug * [TW-162] Fix the bug * Add OutBoundQueueSpec to hspec-discovery * [TW-162] mark more aggressively currentlyInFlight as being a debug function * Try to avoid conflict on .gitignore * [TW-162] Address review's feedback

dniku added 5 commits March 13, 2017 00:07

[CSM-25] Check permissions for wallet key file

bb145e0

[CSM-25] Style improvements

d96b856

[CSM-25] Style improvements

42089e6

[CSM-25] Write user secret file with mode 600

99db139

[CSM-25] Fix potential bug

02e292f

dniku self-assigned this Mar 13, 2017

dniku requested a review from akegalj March 13, 2017 12:55

dniku requested a review from flyingleafe March 13, 2017 12:59

akegalj suggested changes Mar 13, 2017

View reviewed changes

dniku added 2 commits March 13, 2017 20:35

[CSM-25] Fix for file mode check

6989f86

Merge branch 'master' into feature/csm25-wallet-key-permissions

1095eb6

startling reviewed Mar 13, 2017

View reviewed changes

akegalj requested a review from vincenthz March 14, 2017 12:05

Merge branch 'master' into feature/csm25-wallet-key-permissions

1d2a0e8

vincenthz reviewed Mar 14, 2017

View reviewed changes

dniku added 6 commits March 14, 2017 23:11

[CSM-25] Improve error reporting

940a8dd

[CSM-25] Open file in binary mode

f8c11d6

Merge branch 'master' into feature/csm25-wallet-key-permissions

d966d82

Merge branch 'master' into feature/csm25-wallet-key-permissions

9fdc3e5

[CSM-25] Use MonadThrow instead of fail

38b3dfd

[CSM-25] Improve cardano-sl.cabal indentation

2e227bb

neongreen reviewed Mar 19, 2017

View reviewed changes

neongreen suggested changes Mar 19, 2017

View reviewed changes

dniku added 2 commits March 20, 2017 21:02

[CSM-25] Introduce review suggestions

11ed6b8

Merge branch 'master' into feature/csm25-wallet-key-permissions

6cab1d5

dniku added 2 commits March 22, 2017 12:50

[CSM-25] Style improvements

37e62d6

[CSM-25] Style improvements

0e39ff6

vincenthz approved these changes Mar 23, 2017

View reviewed changes

dniku added 2 commits March 27, 2017 14:17

Merge branch 'master' into feature/csm25-wallet-key-permissions

e511415

[CSM-25] Fix warning

890abbc

neongreen approved these changes Mar 27, 2017

View reviewed changes

flyingleafe approved these changes Mar 27, 2017

View reviewed changes

akegalj approved these changes Mar 28, 2017

View reviewed changes

domenkozar reviewed Mar 28, 2017

View reviewed changes

[CSM-25] Fix build on windows

f296dda

akegalj added 3 commits March 28, 2017 15:33

Merge branch 'master' into feature/csm25-wallet-key-permissions

0892ff0

[CSM-25] Fix imports on windows

e0b8d16

Merge remote-tracking branch 'origin/master' into feature/csm25-walle…

d998866

…t-key-permissions

akegalj merged commit 7fd8046 into master Mar 29, 2017

akegalj deleted the feature/csm25-wallet-key-permissions branch March 29, 2017 14:18

dniku changed the title ~~WIP [CSM-25] Wallet secret key file mode~~ [CSM-25] Wallet secret key file mode Apr 10, 2017

ksaric added a commit that referenced this pull request Jul 12, 2017

Merge pull request #98 from input-output-hk/fix/cse98-clone-merge

be56306

[CSE-98] Clone merge

[CSM-25] Wallet secret key file mode #98

[CSM-25] Wallet secret key file mode #98

Conversation

dniku commented Mar 13, 2017

mention-bot commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dniku Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincenthz Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

dniku Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akegalj Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen left a comment

Choose a reason for hiding this comment

vincenthz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenkozar commented Mar 28, 2017 • edited Loading

akegalj commented Mar 28, 2017

dniku Mar 13, 2017 •

edited

Loading

vincenthz Mar 14, 2017 •

edited

Loading

dniku Mar 14, 2017 •

edited

Loading

akegalj Mar 14, 2017 •

edited

Loading

neongreen Mar 19, 2017 •

edited

Loading

domenkozar commented Mar 28, 2017 •

edited

Loading