From c8559a04031e55f61adc693265ab9bd3e8fc2a94 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 15 Jun 2018 14:44:32 -0400 Subject: [PATCH 01/41] close old bug --- doc/bugs/importfeed_does_not_work_with_socks_proxy.mdwn | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/bugs/importfeed_does_not_work_with_socks_proxy.mdwn b/doc/bugs/importfeed_does_not_work_with_socks_proxy.mdwn index cee34367e8..4f892469dc 100644 --- a/doc/bugs/importfeed_does_not_work_with_socks_proxy.mdwn +++ b/doc/bugs/importfeed_does_not_work_with_socks_proxy.mdwn @@ -3,3 +3,6 @@ It appears that `git annex importfeed` can not be used in with socks proxies bec For `addurl`, I could configure the system to use a socks proxy by setting `git config annex.web-download-command "curl --silent --preproxy socks4a://localhost:1080 %url -o %file"`; for `importfeed`, I found no option to override the command used to fetch the URL, and `wget` [lacks SOCKS support](https://savannah.gnu.org/bugs/?func=detailitem&item_id=43576). Please consider using `web-download-command` for `importfeed` too, introducing a dedicated option `web-get-command` (that would output to stdout rather than %file), or otherwise supporting operation behind a SOCKS proxy. + +> Closing this, since the http library git-annex uses supports socks +> proxies via environment settings, as far as I know. [[done]] --[[Joey]] From 28720c795ff57a55b48e56d15f9b6bcb977f48d9 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 15 Jun 2018 16:52:24 -0400 Subject: [PATCH 02/41] limit url downloads to whitelisted schemes Security fix! Allowing any schemes, particularly file: and possibly others like scp: allowed file exfiltration by anyone who had write access to the git repository, since they could add an annexed file using such an url, or using an url that redirected to such an url, and wait for the victim to get it into their repository and send them a copy. * Added annex.security.allowed-url-schemes setting, which defaults to only allowing http and https URLs. Note especially that file:/ is no longer enabled by default. * Removed annex.web-download-command, since its interface does not allow supporting annex.security.allowed-url-schemes across redirects. If you used this setting, you may want to instead use annex.web-options to pass options to curl. With annex.web-download-command removed, nearly all url accesses in git-annex are made via Utility.Url via http-client or curl. http-client only supports http and https, so no problem there. (Disabling one and not the other is not implemented.) Used curl --proto to limit the allowed url schemes. Note that this will cause git annex fsck --from web to mark files using a disallowed url scheme as not being present in the web. That seems acceptable; fsck --from web also does that when a web server is not available. youtube-dl already disabled file: itself (probably for similar reasons). The scheme check was also added to youtube-dl urls for completeness, although that check won't catch any redirects it might follow. But youtube-dl goes off and does its own thing with other protocols anyway, so that's fine. Special remotes that support other domain-specific url schemes are not affected by this change. In the bittorrent remote, aria2c can still download magnet: links. The download of the .torrent file is otherwise now limited by annex.security.allowed-url-schemes. This does not address any external special remotes that might download an url themselves. Current thinking is all external special remotes will need to be audited for this problem, although many of them will use http libraries that only support http and not curl's menagarie. The related problem of accessing private localhost and LAN urls is not addressed by this commit. This commit was sponsored by Brett Eisenberg on Patreon. --- Annex/Content.hs | 14 +---- Annex/Url.hs | 1 + Annex/YoutubeDl.hs | 51 ++++++++++------- CHANGELOG | 12 ++++ Command/AddUrl.hs | 2 +- NEWS | 11 ++++ Test.hs | 6 +- Types/GitConfig.hs | 8 ++- Utility/Url.hs | 56 +++++++++++++++---- doc/git-annex.mdwn | 16 ++++-- .../external/git-annex-remote-torrent | 4 +- doc/special_remotes/web.mdwn | 2 +- ..._333141cc9ec6c26ffd19aa95303a91e3._comment | 3 + ...Git-annex_as_a_web_browsing_assistant.mdwn | 6 -- ..._74167f9fff400f148916003468c77de4._comment | 8 --- ..._to_reproduce_normal_download_command.mdwn | 7 +++ 16 files changed, 139 insertions(+), 68 deletions(-) delete mode 100644 doc/tips/Using_Git-annex_as_a_web_browsing_assistant/comment_1_74167f9fff400f148916003468c77de4._comment diff --git a/Annex/Content.hs b/Annex/Content.hs index d14bc06428..9f3722c609 100644 --- a/Annex/Content.hs +++ b/Annex/Content.hs @@ -943,18 +943,8 @@ downloadUrl k p urls file = -- Poll the file to handle configurations where an external -- download command is used. meteredFile file (Just p) k $ - go =<< annexWebDownloadCommand <$> Annex.getGitConfig - where - go Nothing = Url.withUrlOptions $ \uo -> - liftIO $ anyM (\u -> Url.download p u file uo) urls - go (Just basecmd) = anyM (downloadcmd basecmd) urls - downloadcmd basecmd url = - progressCommand "sh" [Param "-c", Param $ gencmd url basecmd] - <&&> liftIO (doesFileExist file) - gencmd url = massReplace - [ ("%file", shellEscape file) - , ("%url", shellEscape url) - ] + Url.withUrlOptions $ \uo -> + liftIO $ anyM (\u -> Url.download p u file uo) urls {- Copies a key's content, when present, to a temp file. - This is used to speed up some rsyncs. -} diff --git a/Annex/Url.hs b/Annex/Url.hs index 0d65a3eb27..65f53d82d9 100644 --- a/Annex/Url.hs +++ b/Annex/Url.hs @@ -39,6 +39,7 @@ getUrlOptions = Annex.getState Annex.urloptions >>= \case <*> headers <*> options <*> liftIO (U.newManager U.managerSettings) + <*> (annexAllowedUrlSchemes <$> Annex.getGitConfig) headers = annexHttpHeadersCommand <$> Annex.getGitConfig >>= \case Just cmd -> lines <$> liftIO (readProcess "sh" ["-c", cmd]) Nothing -> annexHttpHeaders <$> Annex.getGitConfig diff --git a/Annex/YoutubeDl.hs b/Annex/YoutubeDl.hs index 425d06dfb4..4f8e699c23 100644 --- a/Annex/YoutubeDl.hs +++ b/Annex/YoutubeDl.hs @@ -11,7 +11,7 @@ module Annex.YoutubeDl ( youtubeDlSupported, youtubeDlCheck, youtubeDlFileName, - youtubeDlFileName', + youtubeDlFileNameHtmlOnly, ) where import Annex.Common @@ -41,8 +41,11 @@ import Control.Concurrent.Async -- (Note that we can't use --output to specifiy the file to download to, -- due to ) youtubeDl :: URLString -> FilePath -> Annex (Either String (Maybe FilePath)) -youtubeDl url workdir - | supportedScheme url = ifM (liftIO $ inPath "youtube-dl") +youtubeDl url workdir = withUrlOptions $ youtubeDl' url workdir + +youtubeDl' :: URLString -> FilePath -> UrlOptions -> Annex (Either String (Maybe FilePath)) +youtubeDl' url workdir uo + | supportedScheme uo url = ifM (liftIO $ inPath "youtube-dl") ( runcmd >>= \case Right True -> workdirfiles >>= \case (f:[]) -> return (Right (Just f)) @@ -134,8 +137,11 @@ youtubeDlSupported url = either (const False) id <$> youtubeDlCheck url -- Check if youtube-dl can find media in an url. youtubeDlCheck :: URLString -> Annex (Either String Bool) -youtubeDlCheck url - | supportedScheme url = catchMsgIO $ htmlOnly url False $ do +youtubeDlCheck = withUrlOptions . youtubeDlCheck' + +youtubeDlCheck' :: URLString -> UrlOptions -> Annex (Either String Bool) +youtubeDlCheck' url uo + | supportedScheme uo url = catchMsgIO $ htmlOnly url False $ do opts <- youtubeDlOpts [ Param url, Param "--simulate" ] liftIO $ snd <$> processTranscript "youtube-dl" (toCommand opts) Nothing | otherwise = return (Right False) @@ -144,18 +150,22 @@ youtubeDlCheck url -- -- (This is not always identical to the filename it uses when downloading.) youtubeDlFileName :: URLString -> Annex (Either String FilePath) -youtubeDlFileName url - | supportedScheme url = flip catchIO (pure . Left . show) $ - htmlOnly url nomedia (youtubeDlFileName' url) - | otherwise = return nomedia +youtubeDlFileName url = withUrlOptions go where + go uo + | supportedScheme uo url = flip catchIO (pure . Left . show) $ + htmlOnly url nomedia (youtubeDlFileNameHtmlOnly' url uo) + | otherwise = return nomedia nomedia = Left "no media in url" -- Does not check if the url contains htmlOnly; use when that's already -- been verified. -youtubeDlFileName' :: URLString -> Annex (Either String FilePath) -youtubeDlFileName' url - | supportedScheme url = flip catchIO (pure . Left . show) go +youtubeDlFileNameHtmlOnly :: URLString -> Annex (Either String FilePath) +youtubeDlFileNameHtmlOnly = withUrlOptions . youtubeDlFileNameHtmlOnly' + +youtubeDlFileNameHtmlOnly' :: URLString -> UrlOptions -> Annex (Either String FilePath) +youtubeDlFileNameHtmlOnly' url uo + | supportedScheme uo url = flip catchIO (pure . Left . show) go | otherwise = return nomedia where go = do @@ -189,12 +199,13 @@ youtubeDlOpts addopts = do opts <- map Param . annexYoutubeDlOptions <$> Annex.getGitConfig return (opts ++ addopts) -supportedScheme :: URLString -> Bool -supportedScheme url = case uriScheme <$> parseURIRelaxed url of +supportedScheme :: UrlOptions -> URLString -> Bool +supportedScheme uo url = case parseURIRelaxed url of Nothing -> False - -- avoid ugly message from youtube-dl about not supporting file: - Just "file:" -> False - -- ftp indexes may look like html pages, and there's no point - -- involving youtube-dl in a ftp download - Just "ftp:" -> False - Just _ -> True + Just u -> case uriScheme u of + -- avoid ugly message from youtube-dl about not supporting file: + "file:" -> False + -- ftp indexes may look like html pages, and there's no point + -- involving youtube-dl in a ftp download + "ftp:" -> False + _ -> allowedScheme uo u diff --git a/CHANGELOG b/CHANGELOG index 6e14024bf7..cfa836c4c3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,15 @@ +git-annex (6.20180622) UNRELEASED; urgency=high + + * Added annex.security.allowed-url-schemes setting, which defaults + to only allowing http and https URLs. Note especially that file:/ + is no longer enabled by default. This is a security fix. + * Removed annex.web-download-command, since its interface does not allow + supporting annex.security.allowed-url-schemes across redirects. + If you used this setting, you may want to instead use annex.web-options + to pass options to curl. + + -- Joey Hess Wed, 30 May 2018 11:49:08 -0400 + git-annex (6.20180530) UNRELEASED; urgency=medium * Fix build with ghc 8.4+, which broke due to the Semigroup Monoid change. diff --git a/Command/AddUrl.hs b/Command/AddUrl.hs index 4721599248..156dbafcd4 100644 --- a/Command/AddUrl.hs +++ b/Command/AddUrl.hs @@ -277,7 +277,7 @@ downloadWeb o url urlinfo file = -- Ask youtube-dl what filename it will download -- first, and check if that is already an annexed file, -- to avoid unnecessary work in that case. - | otherwise = youtubeDlFileName' url >>= \case + | otherwise = youtubeDlFileNameHtmlOnly url >>= \case Right dest -> ifAnnexed dest (alreadyannexed dest) (dl dest) diff --git a/NEWS b/NEWS index a1272192d3..ce6f481af7 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,14 @@ +git-annex (6.20180622) upstream; urgency=high + + A security fix has changed git-annex to only support http and https + URL schemes by default. You can enable other URL schemes, at your own risk, + using annex.security.allowed-url-schemes. + + The annex.web-download-command configuration has been removed, + use annex.web-options instead. + + -- Joey Hess Fri, 15 Jun 2018 17:54:23 -0400 + git-annex (6.20180309) upstream; urgency=medium Note that, due to not using rsync to transfer files over ssh diff --git a/Test.hs b/Test.hs index 659ebf17e1..463f06e4dd 100644 --- a/Test.hs +++ b/Test.hs @@ -1714,10 +1714,12 @@ test_add_subdirs = intmpclonerepo $ do test_addurl :: Assertion test_addurl = intmpclonerepo $ do -- file:// only; this test suite should not hit the network + let filecmd c ps = git_annex c ("-cannex.security.allowed-url-schemes=file" : ps) f <- absPath "myurl" let url = replace "\\" "/" ("file:///" ++ dropDrive f) writeFile f "foo" - git_annex "addurl" [url] @? ("addurl failed on " ++ url) + not <$> git_annex "addurl" [url] @? "addurl failed to fail on file url" + filecmd "addurl" [url] @? ("addurl failed on " ++ url) let dest = "addurlurldest" - git_annex "addurl" ["--file", dest, url] @? ("addurl failed on " ++ url ++ " with --file") + filecmd "addurl" ["--file", dest, url] @? ("addurl failed on " ++ url ++ " with --file") doesFileExist dest @? (dest ++ " missing after addurl --file") diff --git a/Types/GitConfig.hs b/Types/GitConfig.hs index 508d02f4d9..e7e9305801 100644 --- a/Types/GitConfig.hs +++ b/Types/GitConfig.hs @@ -33,8 +33,10 @@ import Config.DynamicConfig import Utility.HumanTime import Utility.Gpg (GpgCmd, mkGpgCmd) import Utility.ThreadScheduler (Seconds(..)) +import Utility.Url (Scheme, mkScheme) import Control.Concurrent.STM +import qualified Data.Set as S -- | A configurable value, that may not be fully determined yet because -- the global git config has not yet been loaded. @@ -71,7 +73,6 @@ data GitConfig = GitConfig , annexWebOptions :: [String] , annexYoutubeDlOptions :: [String] , annexAriaTorrentOptions :: [String] - , annexWebDownloadCommand :: Maybe String , annexCrippledFileSystem :: Bool , annexLargeFiles :: Maybe String , annexAddSmallFiles :: Bool @@ -93,6 +94,7 @@ data GitConfig = GitConfig , annexSecureHashesOnly :: Bool , annexRetry :: Maybe Integer , annexRetryDelay :: Maybe Seconds + , annexAllowedUrlSchemes :: S.Set Scheme , coreSymlinks :: Bool , coreSharedRepository :: SharedRepository , receiveDenyCurrentBranch :: DenyCurrentBranch @@ -133,7 +135,6 @@ extractGitConfig r = GitConfig , annexWebOptions = getwords (annex "web-options") , annexYoutubeDlOptions = getwords (annex "youtube-dl-options") , annexAriaTorrentOptions = getwords (annex "aria-torrent-options") - , annexWebDownloadCommand = getmaybe (annex "web-download-command") , annexCrippledFileSystem = getbool (annex "crippledfilesystem") False , annexLargeFiles = getmaybe (annex "largefiles") , annexAddSmallFiles = getbool (annex "addsmallfiles") True @@ -159,6 +160,9 @@ extractGitConfig r = GitConfig , annexRetry = getmayberead (annex "retry") , annexRetryDelay = Seconds <$> getmayberead (annex "retrydelay") + , annexAllowedUrlSchemes = S.fromList $ map mkScheme $ + maybe ["http", "https"] words $ + getmaybe (annex "security.allowed-url-schemes") , coreSymlinks = getbool "core.symlinks" True , coreSharedRepository = getSharedRepository r , receiveDenyCurrentBranch = getDenyCurrentBranch r diff --git a/Utility/Url.hs b/Utility/Url.hs index d959ba023e..e094518ddc 100644 --- a/Utility/Url.hs +++ b/Utility/Url.hs @@ -15,6 +15,9 @@ module Utility.Url ( managerSettings, URLString, UserAgent, + Scheme, + mkScheme, + allowedScheme, UrlOptions(..), defUrlOptions, mkUrlOptions, @@ -41,6 +44,7 @@ import qualified Data.CaseInsensitive as CI import qualified Data.ByteString as B import qualified Data.ByteString.UTF8 as B8 import qualified Data.ByteString.Lazy as L +import qualified Data.Set as S import Control.Monad.Trans.Resource import Network.HTTP.Conduit import Network.HTTP.Client (brRead, withResponse) @@ -65,12 +69,22 @@ type Headers = [String] type UserAgent = String +newtype Scheme = Scheme (CI.CI String) + deriving (Eq, Ord) + +mkScheme :: String -> Scheme +mkScheme = Scheme . CI.mk + +fromScheme :: Scheme -> String +fromScheme (Scheme s) = CI.original s + data UrlOptions = UrlOptions { userAgent :: Maybe UserAgent , reqHeaders :: Headers , urlDownloader :: UrlDownloader , applyRequest :: Request -> Request , httpManager :: Manager + , allowedSchemes :: S.Set Scheme } data UrlDownloader @@ -84,8 +98,9 @@ defUrlOptions = UrlOptions <*> pure DownloadWithConduit <*> pure id <*> newManager managerSettings + <*> pure (S.fromList $ map mkScheme ["http", "https"]) -mkUrlOptions :: Maybe UserAgent -> Headers -> [CommandParam] -> Manager -> UrlOptions +mkUrlOptions :: Maybe UserAgent -> Headers -> [CommandParam] -> Manager -> S.Set Scheme -> UrlOptions mkUrlOptions defuseragent reqheaders reqparams manager = UrlOptions useragent reqheaders urldownloader applyrequest manager where @@ -115,7 +130,7 @@ mkUrlOptions defuseragent reqheaders reqparams manager = _ -> (h', B8.fromString v) curlParams :: UrlOptions -> [CommandParam] -> [CommandParam] -curlParams uo ps = ps ++ uaparams ++ headerparams ++ addedparams +curlParams uo ps = ps ++ uaparams ++ headerparams ++ addedparams ++ schemeparams where uaparams = case userAgent uo of Nothing -> [] @@ -124,6 +139,25 @@ curlParams uo ps = ps ++ uaparams ++ headerparams ++ addedparams addedparams = case urlDownloader uo of DownloadWithConduit -> [] DownloadWithCurl l -> l + schemeparams = + [ Param "--proto" + , Param $ intercalate "," ("-all" : schemelist) + ] + schemelist = map fromScheme $ S.toList $ allowedSchemes uo + +checkPolicy :: UrlOptions -> URI -> a -> IO a -> IO a +checkPolicy uo u onerr a + | allowedScheme uo u = a + | otherwise = do + hPutStrLn stderr $ + "Configuration does not allow accessing " ++ show u + hFlush stderr + return onerr + +allowedScheme :: UrlOptions -> URI -> Bool +allowedScheme uo u = uscheme `S.member` allowedSchemes uo + where + uscheme = mkScheme $ takeWhile (/=':') (uriScheme u) {- Checks that an url exists and could be successfully downloaded, - also checking that its size, if available, matches a specified size. -} @@ -158,7 +192,8 @@ assumeUrlExists = UrlInfo True Nothing Nothing - also returning its size and suggested filename if available. -} getUrlInfo :: URLString -> UrlOptions -> IO UrlInfo getUrlInfo url uo = case parseURIRelaxed url of - Just u -> case (urlDownloader uo, parseUrlConduit (show u)) of + Just u -> checkPolicy uo u dne $ + case (urlDownloader uo, parseUrlConduit (show u)) of (DownloadWithConduit, Just req) -> catchJust -- When http redirects to a protocol which -- conduit does not support, it will throw @@ -166,7 +201,7 @@ getUrlInfo url uo = case parseURIRelaxed url of (matchStatusCodeException (== found302)) (existsconduit req) (const (existscurl u)) - `catchNonAsync` (const dne) + `catchNonAsync` (const $ return dne) -- http-conduit does not support file:, ftp:, etc urls, -- so fall back to reading files and using curl. _ @@ -177,11 +212,11 @@ getUrlInfo url uo = case parseURIRelaxed url of Just stat -> do sz <- getFileSize' f stat found (Just sz) Nothing - Nothing -> dne + Nothing -> return dne | otherwise -> existscurl u - Nothing -> dne + Nothing -> return dne where - dne = return $ UrlInfo False Nothing Nothing + dne = UrlInfo False Nothing Nothing found sz f = return $ UrlInfo True sz f curlparams = curlParams uo $ @@ -213,7 +248,7 @@ getUrlInfo url uo = case parseURIRelaxed url of then found (extractlen resp) (extractfilename resp) - else dne + else return dne existscurl u = do output <- catchDefaultIO "" $ @@ -230,7 +265,7 @@ getUrlInfo url uo = case parseURIRelaxed url of -- don't try to parse ftp status codes; if curl -- got a length, it's good _ | isftp && isJust len -> good - _ -> dne + _ -> return dne -- Parse eg: attachment; filename="fname.ext" -- per RFC 2616 @@ -265,7 +300,8 @@ download meterupdate url file uo = `catchNonAsync` showerr where go = case parseURIRelaxed url of - Just u -> case (urlDownloader uo, parseUrlConduit (show u)) of + Just u -> checkPolicy uo u False $ + case (urlDownloader uo, parseUrlConduit (show u)) of (DownloadWithConduit, Just req) -> catchJust -- When http redirects to a protocol which -- conduit does not support, it will throw diff --git a/doc/git-annex.mdwn b/doc/git-annex.mdwn index f829f07f63..756ba5a021 100644 --- a/doc/git-annex.mdwn +++ b/doc/git-annex.mdwn @@ -1387,12 +1387,20 @@ Here are all the supported configuration settings. If set, the command is run and each line of its output is used as a HTTP header. This overrides annex.http-headers. -* `annex.web-download-command` +* `annex.security.allowed-url-schemes` - Use to specify a command to run to download a file from the web. + List of URL schemes that git-annex is allowed to download content from. + The default is "http https". - In the command line, %url is replaced with the url to download, - and %file is replaced with the file that it should be saved to. + Think very carefully before changing this; there are security + implications. For example, if it's changed to allow "file" URLs, + then anyone who can get a commit into your git-annex repository + could add a pointer to a private file located outside that repository, + risking it being copied into the repository and transferred on to other + remotes, exposing its content. + + Some special remotes support their own domain-specific URL + schemes; those are not affected by this configuration setting. * `annex.secure-erase-command` diff --git a/doc/special_remotes/external/git-annex-remote-torrent b/doc/special_remotes/external/git-annex-remote-torrent index d7897d7726..a3b2bb8580 100755 --- a/doc/special_remotes/external/git-annex-remote-torrent +++ b/doc/special_remotes/external/git-annex-remote-torrent @@ -123,7 +123,7 @@ while read line; do url="$2" # List contents of torrent. tmp=$(mktemp) - if ! runcmd curl -o "$tmp" "$url"; then + if ! runcmd curl --proto -all,http,https -o "$tmp" "$url"; then echo CHECKURL-FAILURE else oldIFS="$IFS" @@ -166,7 +166,7 @@ while read line; do echo TRANSFER-FAILURE RETRIEVE "$key" "no known torrent urls for this key" else tmp=$(mktemp) - if ! runcmd curl -o "$tmp" "$url"; then + if ! runcmd curl --proto -all,http,https -o "$tmp" "$url"; then echo TRANSFER-FAILURE RETRIEVE "$key" "failed downloading torrent file from $url" else filenum="$(echo "$url" | sed 's/(.*#\(\d*\)/\1/')" diff --git a/doc/special_remotes/web.mdwn b/doc/special_remotes/web.mdwn index cd20a93bb1..8bb4091704 100644 --- a/doc/special_remotes/web.mdwn +++ b/doc/special_remotes/web.mdwn @@ -6,6 +6,6 @@ See [[tips/using_the_web_as_a_special_remote]] for usage examples. Currently git-annex only supports downloading content from the web; it cannot upload to it or remove content. -This special remote uses arbitrary urls on the web as the source for content. +This special remote uses urls on the web as the source for content. git-annex can also download content from a normal git remote, accessible by http. diff --git a/doc/special_remotes/web/comment_2_333141cc9ec6c26ffd19aa95303a91e3._comment b/doc/special_remotes/web/comment_2_333141cc9ec6c26ffd19aa95303a91e3._comment index ff20181175..456186a67d 100644 --- a/doc/special_remotes/web/comment_2_333141cc9ec6c26ffd19aa95303a91e3._comment +++ b/doc/special_remotes/web/comment_2_333141cc9ec6c26ffd19aa95303a91e3._comment @@ -5,4 +5,7 @@ date="2013-08-17T08:59:11Z" content=""" When it says \"arbitrary urls\", it means it. The only requirement is that the url be well formed and that wget or whatever command you have it configured to use via annex.web-download-command knows how to download it. + +Update 2018: That used to be the case, but it's now limited by default to +http and https urls. """]] diff --git a/doc/tips/Using_Git-annex_as_a_web_browsing_assistant.mdwn b/doc/tips/Using_Git-annex_as_a_web_browsing_assistant.mdwn index 4ee023de3e..e8239abd40 100644 --- a/doc/tips/Using_Git-annex_as_a_web_browsing_assistant.mdwn +++ b/doc/tips/Using_Git-annex_as_a_web_browsing_assistant.mdwn @@ -11,12 +11,6 @@ The first step is to install the Firefox plugin [FlashGot](http://flashgot.net/). We will use it to provide the Firefox shortcuts to add things to our annex. -We also need a normal download manager, if we want to get status updates as -the download is done. We'll need to configure git-annex to use it by -setting `annex.web-download-command` as Joey describes in his comment on -[[todo/wishlist: allow configuration of downloader for addurl]]. See the -manpage [[git-annex]] for more information on setting configuration. - Once we have installed all that, we need a script that has an interface which FlashGot can treat as a downloader, but which calls git-annex to do the actual downloading. Such a script is available from diff --git a/doc/tips/Using_Git-annex_as_a_web_browsing_assistant/comment_1_74167f9fff400f148916003468c77de4._comment b/doc/tips/Using_Git-annex_as_a_web_browsing_assistant/comment_1_74167f9fff400f148916003468c77de4._comment deleted file mode 100644 index 4c5c3cdfaf..0000000000 --- a/doc/tips/Using_Git-annex_as_a_web_browsing_assistant/comment_1_74167f9fff400f148916003468c77de4._comment +++ /dev/null @@ -1,8 +0,0 @@ -[[!comment format=mdwn - username="http://joeyh.name/" - nickname="joey" - subject="comment 1" - date="2013-04-11T20:16:02Z" - content=""" -As of my last commit, you don't really need a separate download manager. The webapp will now display urls that `git annex addurl` is downloading in among the other transfers. -"""]] diff --git a/doc/todo/easy_way_to_reproduce_normal_download_command.mdwn b/doc/todo/easy_way_to_reproduce_normal_download_command.mdwn index 6846bc8a19..5e913a544d 100644 --- a/doc/todo/easy_way_to_reproduce_normal_download_command.mdwn +++ b/doc/todo/easy_way_to_reproduce_normal_download_command.mdwn @@ -22,3 +22,10 @@ what about the other settings, is it okay to hardcode those? maybe this would be easier if there would be an options override just like rsync, but separate ones for curl and wget... --[[anarcat]] + +> git-annex now only uses curl, and defaults to a built-in http downloader. +> The annex.web-download-command is no longer supported. annex.web-options +> can be used to pass options to curl. +> +> So, I don't think this todo is relevant anymore, closing [[done]]. +> --[[Joey]] From 40e835828444c589aa6a5d89269b2c866ff66534 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sat, 16 Jun 2018 14:18:29 -0400 Subject: [PATCH 03/41] add Utility.HttpManagerRestricted This is a clean way to add IP address restrictions to http-client, and any library using it. See https://github.com/snoyberg/http-client/issues/354#issuecomment-397830259 Some code from http-client and http-client-tls was copied in and modified. Credited its author accordingly, and used the same MIT license. The restrictions don't apply to http proxies. If using http proxies is a problem, http-client already has a way to disable them. SOCKS support is not included. As far as I can tell, http-client-tls does not support SOCKS by default, and so git-annex never has. The additional dependencies are free; git-annex already transitively depended on them via http-conduit. This commit was sponsored by Eric Drechsel on Patreon. --- COPYRIGHT | 46 ++++++----- Utility/HttpManagerRestricted.hs | 132 +++++++++++++++++++++++++++++++ git-annex.cabal | 3 + 3 files changed, 161 insertions(+), 20 deletions(-) create mode 100644 Utility/HttpManagerRestricted.hs diff --git a/COPYRIGHT b/COPYRIGHT index cf08323b96..2b19d183b9 100644 --- a/COPYRIGHT +++ b/COPYRIGHT @@ -24,6 +24,11 @@ Copyright: 2011 Bas van Dijk & Roel van Dijk 2012, 2013 Joey Hess License: BSD-2-clause +Files: Utility/HttpManagerRestricted.hs +Copyright: 2018 Joey Hess + 2013 Michael Snoyman +License: MIT + Files: Utility/* Copyright: 2012-2018 Joey Hess License: BSD-2-clause @@ -51,26 +56,7 @@ Copyright: © 2005-2011 by John Resig, Branden Aaron & Jörn Zaefferer License: MIT or GPL-2 The full text of version 2 of the GPL is distributed in /usr/share/common-licenses/GPL-2 on Debian systems. The text of the MIT - license follows: - . - Permission is hereby granted, free of charge, to any person obtaining - a copy of this software and associated documentation files (the - "Software"), to deal in the Software without restriction, including - without limitation the rights to use, copy, modify, merge, publish, - distribute, sublicense, and/or sell copies of the Software, and to - permit persons to whom the Software is furnished to do so, subject to - the following conditions: - . - The above copyright notice and this permission notice shall be - included in all copies or substantial portions of the Software. - . - THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, - EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF - MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND - NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE - LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION - OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION - WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + license is in the MIT section below. Files: static/*/bootstrap* static/*/glyphicons-halflings* Copyright: 2012-2014 Twitter, Inc. @@ -153,6 +139,26 @@ License: BSD-2-clause LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +License: MIT + Permission is hereby granted, free of charge, to any person obtaining + a copy of this software and associated documentation files (the + "Software"), to deal in the Software without restriction, including + without limitation the rights to use, copy, modify, merge, publish, + distribute, sublicense, and/or sell copies of the Software, and to + permit persons to whom the Software is furnished to do so, subject to + the following conditions: + . + The above copyright notice and this permission notice shall be + included in all copies or substantial portions of the Software. + . + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE + LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION + OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. License: AGPL-3+ GNU AFFERO GENERAL PUBLIC LICENSE diff --git a/Utility/HttpManagerRestricted.hs b/Utility/HttpManagerRestricted.hs new file mode 100644 index 0000000000..2b1e241cc4 --- /dev/null +++ b/Utility/HttpManagerRestricted.hs @@ -0,0 +1,132 @@ +{- | Restricted Manager for http-client-tls + - + - Copyright 2018 Joey Hess + - + - Portions from http-client-tls Copyright (c) 2013 Michael Snoyman + - + - License: MIT + -} + +{-# LANGUAGE ScopedTypeVariables, DeriveDataTypeable #-} + +module Utility.HttpManagerRestricted ( + restrictManagerSettings, + Restriction(..), + ConnectionRestricted(..), + addrConnectionRestricted, +) where + +import Network.HTTP.Client +import Network.HTTP.Client.Internal (ManagerSettings(..), Connection) +import Network.HTTP.Client.TLS +import Network.Socket +import Network.BSD (getProtocolNumber) +import Control.Exception +import qualified Network.Connection as NC +import Data.ByteString (ByteString) +import Data.Default +import Data.Typeable + +data Restriction = Restriction + { addressRestriction :: AddrInfo -> Maybe ConnectionRestricted + } + +data ConnectionRestricted = ConnectionRestricted String + deriving (Show, Typeable) + +instance Exception ConnectionRestricted + +addrConnectionRestricted :: AddrInfo -> ConnectionRestricted +addrConnectionRestricted addr = ConnectionRestricted $ unwords + [ "Configuration does not allow accessing address" + , case addrAddress addr of + a@(SockAddrInet _ _) -> + takeWhile (/= ':') $ show a + a@(SockAddrInet6 _ _ _ _) -> + takeWhile (/= ']') $ drop 1 $ show a + ] + +-- | Adjusts a ManagerSettings to check a Restriction. +-- +-- Note that connections to http proxies are not checked. +-- Use `managerSetProxy noProxy` to prevent connections through http +-- proxies. +restrictManagerSettings :: Restriction -> ManagerSettings -> ManagerSettings +restrictManagerSettings cfg base = base + { managerRawConnection = restrictedRawConnection cfg + , managerTlsConnection = restrictedTlsConnection cfg + , managerWrapException = \req -> + let wrapper se + | Just (_ :: ConnectionRestricted) <- fromException se = + toException $ HttpExceptionRequest req $ + InternalException se + | otherwise = se + in handle $ throwIO . wrapper + } + +restrictedRawConnection :: Restriction -> IO (Maybe HostAddress -> String -> Int -> IO Connection) +restrictedRawConnection cfg = getConnection cfg Nothing + +restrictedTlsConnection :: Restriction -> IO (Maybe HostAddress -> String -> Int -> IO Connection) +restrictedTlsConnection cfg = getConnection cfg $ + -- It's not possible to access the TLSSettings + -- used in the base ManagerSettings. So, use the default + -- value, which is the same thing http-client-tls defaults to. + -- Since changing from the default settings can only make TLS + -- less secure, this is not a big problem. + Just def + +-- Based on Network.HTTP.Client.TLS.getTlsConnection. +-- +-- Checks the Restriction +-- +-- Does not support SOCKS. +getConnection :: Restriction -> Maybe NC.TLSSettings -> IO (Maybe HostAddress -> String -> Int -> IO Connection) +getConnection cfg tls = do + context <- NC.initConnectionContext + return $ \_ha host port -> bracketOnError + (go context host port) + NC.connectionClose + convertConnection + where + go context host port = do + let connparams = NC.ConnectionParams + { NC.connectionHostname = host + , NC.connectionPort = fromIntegral port + , NC.connectionUseSecure = tls + , NC.connectionUseSocks = Nothing -- unsupprted + } + proto <- getProtocolNumber "tcp" + let serv = show port + let hints = defaultHints + { addrFlags = [AI_ADDRCONFIG] + , addrProtocol = proto + , addrSocketType = Stream + } + addrs <- getAddrInfo (Just hints) (Just host) (Just serv) + bracketOnError + (firstSuccessful $ map tryToConnect addrs) + close + (\sock -> NC.connectFromSocket context sock connparams) + where + tryToConnect addr = case addressRestriction cfg addr of + Nothing -> bracketOnError + (socket (addrFamily addr) (addrSocketType addr) (addrProtocol addr)) + close + (\sock -> connect sock (addrAddress addr) >> return sock) + Just r -> throwIO r + firstSuccessful [] = throwIO $ NC.HostNotResolved host + firstSuccessful (a:as) = a `catch` \(e ::IOException) -> + case as of + [] -> throwIO e + _ -> firstSuccessful as + +-- Copied from Network.HTTP.Client.TLS, unfortunately not exported. +convertConnection :: NC.Connection -> IO Connection +convertConnection conn = makeConnection + (NC.connectionGetChunk conn) + (NC.connectionPut conn) + -- Closing an SSL connection gracefully involves writing/reading + -- on the socket. But when this is called the socket might be + -- already closed, and we get a @ResourceVanished@. + (NC.connectionClose conn `Control.Exception.catch` \(_ :: IOException) -> return ()) diff --git a/git-annex.cabal b/git-annex.cabal index 627e760783..9d12694ea1 100644 --- a/git-annex.cabal +++ b/git-annex.cabal @@ -340,7 +340,9 @@ Executable git-annex bloomfilter, edit-distance, resourcet, + connection, http-client, + http-client-tls, http-types (>= 0.7), http-conduit (>= 2.0), conduit, @@ -1032,6 +1034,7 @@ Executable git-annex Utility.Gpg Utility.Hash Utility.HtmlDetect + Utility.HttpManagerRestricted Utility.HumanNumber Utility.HumanTime Utility.InodeCache From 014a3fef348650fad5abad53a131b2a4c56d20b3 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sat, 16 Jun 2018 18:44:27 -0400 Subject: [PATCH 04/41] added isPrivateAddress and isLoopbackAddress For use in a security boundary enforcement. Based on https://en.wikipedia.org/wiki/Reserved_IP_addresses Including supporting IPv4 addresses embedded in IPv6 addresses. Because while RFC6052 3.1 says "Address translators MUST NOT translate packets in which an address is composed of the Well-Known Prefix and a non- global IPv4 address; they MUST drop these packets", I don't want to trust that implementations get that right when enforcing a security boundary. This commit was sponsored by John Pellman on Patreon. --- Utility/IPAddress.hs | 71 ++++++++++++++++++++++++++++++++++++++++++++ git-annex.cabal | 1 + 2 files changed, 72 insertions(+) create mode 100644 Utility/IPAddress.hs diff --git a/Utility/IPAddress.hs b/Utility/IPAddress.hs new file mode 100644 index 0000000000..d868639fdd --- /dev/null +++ b/Utility/IPAddress.hs @@ -0,0 +1,71 @@ +{- IP addresses + - + - Copyright 2012 Joey Hess + - + - License: BSD-2-clause + -} + +module Utility.IPAddress where + +import Utility.Exception + +import Network.Socket +import Data.Word +import Control.Applicative +import Prelude + +{- Check if an IP address is a loopback address; connecting to it + - may connect back to the local host. -} +isLoopbackAddress :: SockAddr -> Bool +isLoopbackAddress (SockAddrInet _ ipv4) = case hostAddressToTuple ipv4 of + -- localhost + (127,_,_,_) -> True + -- current network; functions equivilant to loopback + (0,_,_, _) -> True + _ -> False +isLoopbackAddress (SockAddrInet6 _ _ ipv6 _) = case hostAddress6ToTuple ipv6 of + -- localhost + (0,0,0,0,0,0,0,1) -> True + -- unspecified address; functions equivilant to loopback + (0,0,0,0,0,0,0,0) -> True + v -> maybe False + (isLoopbackAddress . SockAddrInet 0) + (embeddedIpv4 v) +isLoopbackAddress _ = False + +{- Check if an IP address is not globally routed, and is used + - for private communication, eg on a LAN. -} +isPrivateAddress :: SockAddr -> Bool +isPrivateAddress (SockAddrInet _ ipv4) = case hostAddressToTuple ipv4 of + -- lan + (10,_,_,_) -> True + (172,n,_,_) | n >= 16 && n <= 31 -> True -- 172.16.0.0/12 + (192,168,_,_) -> True + -- carrier-grade NAT + (100,n,0,0) | n >= 64 && n <= 127 -> True -- 100.64.0.0/10 + -- link-local + (169,254,_,_) -> True + _ -> False +isPrivateAddress (SockAddrInet6 _ _ ipv6 _) = case hostAddress6ToTuple ipv6 of + v@(n,_,_,_,_,_,_,_) + -- local to lan or private between orgs + | n >= 0xfc00 && n <= 0xfdff -> True -- fc00::/7 + -- link-local + | n >= 0xfe80 && n <= 0xfebf -> True -- fe80::/10 + | otherwise -> maybe False + (isPrivateAddress . SockAddrInet 0) + (embeddedIpv4 v) +isPrivateAddress _ = False + +embeddedIpv4 :: (Word16, Word16, Word16, Word16, Word16, Word16, Word16, Word16) -> Maybe HostAddress +embeddedIpv4 v = case v of + -- IPv4 mapped address (::ffff:0:0/96) + (0,0,0,0,0,0xffff,a,b) -> Just (toipv4 a b) + -- IPV4 translated address (::ffff:0:ipv4) + (0,0,0,0,0xffff,0,a,b) -> Just (toipv4 a b) + -- IPV4/IPV6 translation (64:ff9b::ipv4) + (0x64,0xff9b,0,0,0,0,a,b) -> Just (toipv4 a b) + _ -> Nothing + where + toipv4 a b = htonl $ fromIntegral a * (2^halfipv4bits) + fromIntegral b + halfipv4bits = 16 :: Word32 diff --git a/git-annex.cabal b/git-annex.cabal index 9d12694ea1..bab975c26a 100644 --- a/git-annex.cabal +++ b/git-annex.cabal @@ -1038,6 +1038,7 @@ Executable git-annex Utility.HumanNumber Utility.HumanTime Utility.InodeCache + Utility.IPAddress Utility.LinuxMkLibs Utility.LockFile Utility.LockFile.LockStatus From 43bf219a3c60f8880715a9f643218d532086c9d9 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sat, 16 Jun 2018 20:26:36 -0400 Subject: [PATCH 05/41] added makeAddressMatcher Would be nice to add CIDR notation to this, but this is the minimal thing needed for the security fix. This commit was sponsored by Ewen McNeill on Patreon. --- Utility/IPAddress.hs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Utility/IPAddress.hs b/Utility/IPAddress.hs index d868639fdd..3a61dc34a7 100644 --- a/Utility/IPAddress.hs +++ b/Utility/IPAddress.hs @@ -69,3 +69,25 @@ embeddedIpv4 v = case v of where toipv4 a b = htonl $ fromIntegral a * (2^halfipv4bits) + fromIntegral b halfipv4bits = 16 :: Word32 + +{- Given a string containing an IP address, make a function that will + - match that address in a SockAddr. Nothing when the address cannot be + - parsed. + - + - This does not involve any DNS lookups. + -} +makeAddressMatcher :: String -> IO (Maybe (SockAddr -> Bool)) +makeAddressMatcher s = go + <$> catchDefaultIO [] (getAddrInfo (Just hints) (Just s) Nothing) + where + hints = defaultHints + { addrSocketType = Stream + , addrFlags = [AI_NUMERICHOST] + } + + go [] = Nothing + go l = Just $ \sockaddr -> any (match sockaddr) (map addrAddress l) + + match (SockAddrInet _ a) (SockAddrInet _ b) = a == b + match (SockAddrInet6 _ _ a _) (SockAddrInet6 _ _ b _) = a == b + match _ _ = False From b54b2cdc0ef1373fc200c0d28fded3c04fd57212 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sun, 17 Jun 2018 13:05:30 -0400 Subject: [PATCH 06/41] prevent http connections to localhost and private ips by default Security fix! * git-annex will refuse to download content from http servers on localhost, or any private IP addresses, to prevent accidental exposure of internal data. This can be overridden with the annex.security.allowed-http-addresses setting. * Since curl's interface does not have a way to prevent it from accessing localhost or private IP addresses, curl defaults to not being used for url downloads, even if annex.web-options enabled it before. Only when annex.security.allowed-http-addresses=all will curl be used. Since S3 and WebDav use the Manager, the same policies apply to them too. youtube-dl is not handled yet, and a http proxy configuration can bypass these checks too. Those cases are still TBD. This commit was sponsored by Jeff Goeke-Smith on Patreon. --- Annex/Url.hs | 55 ++++++++++++++++++++---- CHANGELOG | 8 ++++ Types/GitConfig.hs | 3 ++ Utility/Url.hs | 103 ++++++++++++++++++++++++--------------------- doc/git-annex.mdwn | 35 ++++++++++++--- 5 files changed, 141 insertions(+), 63 deletions(-) diff --git a/Annex/Url.hs b/Annex/Url.hs index 65f53d82d9..7b6e4ec8f0 100644 --- a/Annex/Url.hs +++ b/Annex/Url.hs @@ -1,5 +1,5 @@ {- Url downloading, with git-annex user agent and configured http - - headers and curl options. + - headers, security restrictions, etc. - - Copyright 2013-2018 Joey Hess - @@ -16,8 +16,12 @@ module Annex.Url ( import Annex.Common import qualified Annex import Utility.Url as U +import Utility.IPAddress +import Utility.HttpManagerRestricted import qualified BuildInfo +import Network.Socket + defaultUserAgent :: U.UserAgent defaultUserAgent = "git-annex/" ++ BuildInfo.packageversion @@ -34,16 +38,51 @@ getUrlOptions = Annex.getState Annex.urloptions >>= \case { Annex.urloptions = Just uo } return uo where - mk = mkUrlOptions - <$> getUserAgent - <*> headers - <*> options - <*> liftIO (U.newManager U.managerSettings) - <*> (annexAllowedUrlSchemes <$> Annex.getGitConfig) + mk = do + (urldownloader, manager) <- checkallowedaddr + mkUrlOptions + <$> getUserAgent + <*> headers + <*> pure urldownloader + <*> pure manager + <*> (annexAllowedUrlSchemes <$> Annex.getGitConfig) + headers = annexHttpHeadersCommand <$> Annex.getGitConfig >>= \case Just cmd -> lines <$> liftIO (readProcess "sh" ["-c", cmd]) Nothing -> annexHttpHeaders <$> Annex.getGitConfig - options = map Param . annexWebOptions <$> Annex.getGitConfig + + checkallowedaddr = words . annexAllowedHttpAddresses <$> Annex.getGitConfig >>= \case + ["all"] -> do + -- Only allow curl when all are allowed, + -- as its interface does not allow preventing + -- it from accessing specific IP addresses. + curlopts <- map Param . annexWebOptions <$> Annex.getGitConfig + let urldownloader = if null curlopts + then U.DownloadWithCurl curlopts + else U.DownloadWithConduit + manager <- liftIO $ U.newManager U.managerSettings + return (urldownloader, manager) + allowedaddrs -> do + addrmatcher <- liftIO $ + (\l v -> any (\f -> f v) l) . catMaybes + <$> mapM makeAddressMatcher allowedaddrs + -- Default to not allowing access to loopback + -- and private IP addresses to avoid data + -- leakage. + let isallowed addr + | addrmatcher addr = True + | isLoopbackAddress addr = False + | isPrivateAddress addr = False + | otherwise = True + let r = Restriction + { addressRestriction = \addr -> + if isallowed (addrAddress addr) + then Nothing + else Just (addrConnectionRestricted addr) + } + manager <- liftIO $ U.newManager $ + restrictManagerSettings r U.managerSettings + return (U.DownloadWithConduit, manager) withUrlOptions :: (U.UrlOptions -> Annex a) -> Annex a withUrlOptions a = a =<< getUrlOptions diff --git a/CHANGELOG b/CHANGELOG index cfa836c4c3..a16f303289 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -7,6 +7,14 @@ git-annex (6.20180622) UNRELEASED; urgency=high supporting annex.security.allowed-url-schemes across redirects. If you used this setting, you may want to instead use annex.web-options to pass options to curl. + * git-annex will refuse to download content from http servers on + localhost, or any private IP addresses, to prevent accidental + exposure of internal data. This can be overridden with the + annex.security.allowed-http-addresses setting. + * Since curl's interface does not have a way to prevent it from accessing + localhost or private IP addresses, curl defaults to not being used + for url downloads, even if annex.web-options enabled it before. + Only when annex.security.allowed-http-addresses=all will curl be used. -- Joey Hess Wed, 30 May 2018 11:49:08 -0400 diff --git a/Types/GitConfig.hs b/Types/GitConfig.hs index e7e9305801..98c8c6e83c 100644 --- a/Types/GitConfig.hs +++ b/Types/GitConfig.hs @@ -95,6 +95,7 @@ data GitConfig = GitConfig , annexRetry :: Maybe Integer , annexRetryDelay :: Maybe Seconds , annexAllowedUrlSchemes :: S.Set Scheme + , annexAllowedHttpAddresses :: String , coreSymlinks :: Bool , coreSharedRepository :: SharedRepository , receiveDenyCurrentBranch :: DenyCurrentBranch @@ -163,6 +164,8 @@ extractGitConfig r = GitConfig , annexAllowedUrlSchemes = S.fromList $ map mkScheme $ maybe ["http", "https"] words $ getmaybe (annex "security.allowed-url-schemes") + , annexAllowedHttpAddresses = fromMaybe "" $ + getmaybe (annex "security.allowed-http-addresses") , coreSymlinks = getbool "core.symlinks" True , coreSharedRepository = getSharedRepository r , receiveDenyCurrentBranch = getDenyCurrentBranch r diff --git a/Utility/Url.hs b/Utility/Url.hs index e094518ddc..5664c47124 100644 --- a/Utility/Url.hs +++ b/Utility/Url.hs @@ -18,6 +18,7 @@ module Utility.Url ( Scheme, mkScheme, allowedScheme, + UrlDownloader(..), UrlOptions(..), defUrlOptions, mkUrlOptions, @@ -37,6 +38,7 @@ module Utility.Url ( import Common import Utility.Metered +import Utility.HttpManagerRestricted import Network.URI import Network.HTTP.Types @@ -100,19 +102,10 @@ defUrlOptions = UrlOptions <*> newManager managerSettings <*> pure (S.fromList $ map mkScheme ["http", "https"]) -mkUrlOptions :: Maybe UserAgent -> Headers -> [CommandParam] -> Manager -> S.Set Scheme -> UrlOptions -mkUrlOptions defuseragent reqheaders reqparams manager = +mkUrlOptions :: Maybe UserAgent -> Headers -> UrlDownloader -> Manager -> S.Set Scheme -> UrlOptions +mkUrlOptions defuseragent reqheaders urldownloader manager = UrlOptions useragent reqheaders urldownloader applyrequest manager where - urldownloader = if null reqparams -#if MIN_VERSION_cryptonite(0,6,0) - then DownloadWithConduit -#else - -- Work around for old cryptonite bug that broke tls. - -- https://github.com/vincenthz/hs-tls/issues/109 - then DownloadWithCurl reqparams -#endif - else DownloadWithCurl reqparams applyrequest = \r -> r { requestHeaders = requestHeaders r ++ addedheaders } addedheaders = uaheader ++ otherheaders useragent = maybe defuseragent (Just . B8.toString . snd) @@ -154,6 +147,12 @@ checkPolicy uo u onerr a hFlush stderr return onerr +unsupportedUrlScheme :: URI -> IO () +unsupportedUrlScheme u = do + hPutStrLn stderr $ + "Unsupported url scheme" ++ show u + hFlush stderr + allowedScheme :: UrlOptions -> URI -> Bool allowedScheme uo u = uscheme `S.member` allowedSchemes uo where @@ -194,31 +193,24 @@ getUrlInfo :: URLString -> UrlOptions -> IO UrlInfo getUrlInfo url uo = case parseURIRelaxed url of Just u -> checkPolicy uo u dne $ case (urlDownloader uo, parseUrlConduit (show u)) of - (DownloadWithConduit, Just req) -> catchJust - -- When http redirects to a protocol which - -- conduit does not support, it will throw - -- a StatusCodeException with found302. - (matchStatusCodeException (== found302)) - (existsconduit req) - (const (existscurl u)) - `catchNonAsync` (const $ return dne) - -- http-conduit does not support file:, ftp:, etc urls, - -- so fall back to reading files and using curl. - _ - | uriScheme u == "file:" -> do - let f = unEscapeString (uriPath u) - s <- catchMaybeIO $ getFileStatus f - case s of - Just stat -> do - sz <- getFileSize' f stat - found (Just sz) Nothing - Nothing -> return dne - | otherwise -> existscurl u + (DownloadWithConduit, Just req) -> + existsconduit req + `catchNonAsync` (const $ return dne) + (DownloadWithConduit, Nothing) + | isfileurl u -> existsfile u + | otherwise -> do + unsupportedUrlScheme u + return dne + (DownloadWithCurl _, _) + | isfileurl u -> existsfile u + | otherwise -> existscurl u Nothing -> return dne where dne = UrlInfo False Nothing Nothing found sz f = return $ UrlInfo True sz f + isfileurl u = uriScheme u == "file:" + curlparams = curlParams uo $ [ Param "-s" , Param "--head" @@ -266,6 +258,15 @@ getUrlInfo url uo = case parseURIRelaxed url of -- got a length, it's good _ | isftp && isJust len -> good _ -> return dne + + existsfile u = do + let f = unEscapeString (uriPath u) + s <- catchMaybeIO $ getFileStatus f + case s of + Just stat -> do + sz <- getFileSize' f stat + found (Just sz) Nothing + Nothing -> return dne -- Parse eg: attachment; filename="fname.ext" -- per RFC 2616 @@ -287,10 +288,6 @@ headRequest r = r } {- Download a perhaps large file, with auto-resume of incomplete downloads. - - - - By default, conduit is used for the download, except for file: urls, - - which are copied. If the url scheme is not supported by conduit, falls - - back to using curl. - - Displays error message on stderr when download failed. -} @@ -302,21 +299,19 @@ download meterupdate url file uo = go = case parseURIRelaxed url of Just u -> checkPolicy uo u False $ case (urlDownloader uo, parseUrlConduit (show u)) of - (DownloadWithConduit, Just req) -> catchJust - -- When http redirects to a protocol which - -- conduit does not support, it will throw - -- a StatusCodeException with found302. - (matchStatusCodeException (== found302)) - (downloadconduit req) - (const downloadcurl) - _ - | uriScheme u == "file:" -> do - let src = unEscapeString (uriPath u) - withMeteredFile src meterupdate $ - L.writeFile file - return True - | otherwise -> downloadcurl + (DownloadWithConduit, Just req) -> + downloadconduit req + (DownloadWithConduit, Nothing) + | isfileurl u -> downloadfile u + | otherwise -> do + unsupportedUrlScheme u + return False + (DownloadWithCurl _, _) + | isfileurl u -> downloadfile u + | otherwise -> downloadcurl Nothing -> return False + + isfileurl u = uriScheme u == "file:" downloadconduit req = catchMaybeIO (getFileSize file) >>= \case Nothing -> runResourceT $ do @@ -369,6 +364,10 @@ download meterupdate url file uo = let msg = case he of HttpExceptionRequest _ (StatusCodeException _ msgb) -> B8.toString msgb + HttpExceptionRequest _ (InternalException ie) -> + case fromException ie of + Nothing -> show ie + Just (ConnectionRestricted why) -> why HttpExceptionRequest _ other -> show other _ -> show he #else @@ -401,6 +400,12 @@ download meterupdate url file uo = , Param "-C", Param "-" ] boolSystem "curl" (ps ++ [Param "-o", File file, File url]) + + downloadfile u = do + let src = unEscapeString (uriPath u) + withMeteredFile src meterupdate $ + L.writeFile file + return True {- Sinks a Response's body to a file. The file can either be opened in - WriteMode or AppendMode. Updates the meter as data is received. diff --git a/doc/git-annex.mdwn b/doc/git-annex.mdwn index 756ba5a021..58f7e09392 100644 --- a/doc/git-annex.mdwn +++ b/doc/git-annex.mdwn @@ -1356,12 +1356,16 @@ Here are all the supported configuration settings. * `annex.web-options` - Setting this makes git-annex use curl to download urls + Options to pass to curl when git-annex uses it to download urls (rather than the default built-in url downloader). For example, to force IPv4 only, set it to "-4". Or to make curl use your ~/.netrc file, set it to "--netrc". + Setting this option makes git-annex use curl, but only + when annex.security.allowed-http-addresses is configured in a + specific way. See its documentation. + * `annex.youtube-dl-options` Options to pass to youtube-dl when using it to find the url to download @@ -1393,15 +1397,34 @@ Here are all the supported configuration settings. The default is "http https". Think very carefully before changing this; there are security - implications. For example, if it's changed to allow "file" URLs, - then anyone who can get a commit into your git-annex repository - could add a pointer to a private file located outside that repository, - risking it being copied into the repository and transferred on to other - remotes, exposing its content. + implications. For example, if it's changed to allow "file" URLs, then + anyone who can get a commit into your git-annex repository could + `git-annex addurl` a pointer to a private file located outside that + repository, possibly causing it to be copied into your repository + and transferred on to other remotes, exposing its content. Some special remotes support their own domain-specific URL schemes; those are not affected by this configuration setting. +* `annex.security.allowed-http-addresses` + + By default, git-annex only makes HTTP connections to public IP addresses; + it will refuse to use HTTP servers on localhost or on a private network. + + This setting can override that behavior, allowing access to particular + IP addresses. For example "127.0.0.1 ::1" allows access to localhost + (both IPV4 and IPV6). To allow access to all IP addresses, use "all" + + Think very carefully before changing this; there are security + implications. Anyone who can get a commit into your git-annex repository + could `git annex addurl` an url on a private http server, possibly + causing it to be downloaded into your repository transferred to + other remotes, exposing its content. + + Note that, since curl's interface does not allow these IP address + restrictions to be enforced, any configuration that enables use of curl + will be ignored unless annex.security.allowed-http-addresses=all. + * `annex.secure-erase-command` This can be set to a command that should be run whenever git-annex From 563f2f5a818ffb85e37dc06856920b63268499e6 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sun, 17 Jun 2018 13:56:17 -0400 Subject: [PATCH 07/41] missed a NEWS update in last commit --- NEWS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS b/NEWS index ce6f481af7..f9f42fc9e3 100644 --- a/NEWS +++ b/NEWS @@ -4,9 +4,17 @@ git-annex (6.20180622) upstream; urgency=high URL schemes by default. You can enable other URL schemes, at your own risk, using annex.security.allowed-url-schemes. + A related security fix prevents git-annex from connecting to http + servers on localhost or private networks. This can be overridden, + at your own risk, using annex.security.allowed-http-addresses. + The annex.web-download-command configuration has been removed, use annex.web-options instead. + Setting annex.web-options no longer is enough to make curl be used. + See the documentation of annex.security.allowed-http-addresses for + why and how to enable curl. + -- Joey Hess Fri, 15 Jun 2018 17:54:23 -0400 git-annex (6.20180309) upstream; urgency=medium From e62c4543c31a61186ebf2e4e0412df59fc8630c8 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sun, 17 Jun 2018 14:46:22 -0400 Subject: [PATCH 08/41] default to not using youtube-dl, for security Pity, but same reasoning as curl applies to it. This commit was sponsored by Peter on Patreon. --- Annex/Url.hs | 5 +++ Annex/YoutubeDl.hs | 36 +++++++++++++++---- CHANGELOG | 9 ++--- NEWS | 9 ++--- doc/git-annex-addurl.mdwn | 9 +++-- doc/git-annex-importfeed.mdwn | 5 ++- doc/git-annex.mdwn | 6 ++-- doc/tips/downloading_podcasts.mdwn | 4 +++ .../using_the_web_as_a_special_remote.mdwn | 4 +++ 9 files changed, 66 insertions(+), 21 deletions(-) diff --git a/Annex/Url.hs b/Annex/Url.hs index 7b6e4ec8f0..c91c54dac3 100644 --- a/Annex/Url.hs +++ b/Annex/Url.hs @@ -11,6 +11,7 @@ module Annex.Url ( withUrlOptions, getUrlOptions, getUserAgent, + httpAddressesUnlimited, ) where import Annex.Common @@ -84,5 +85,9 @@ getUrlOptions = Annex.getState Annex.urloptions >>= \case restrictManagerSettings r U.managerSettings return (U.DownloadWithConduit, manager) +httpAddressesUnlimited :: Annex Bool +httpAddressesUnlimited = + ("all" == ) . annexAllowedHttpAddresses <$> Annex.getGitConfig + withUrlOptions :: (U.UrlOptions -> Annex a) -> Annex a withUrlOptions a = a =<< getUrlOptions diff --git a/Annex/YoutubeDl.hs b/Annex/YoutubeDl.hs index 4f8e699c23..cb4f4e23ff 100644 --- a/Annex/YoutubeDl.hs +++ b/Annex/YoutubeDl.hs @@ -1,6 +1,6 @@ {- youtube-dl integration for git-annex - - - Copyright 2017 Joey Hess + - Copyright 2017-2018 Joey Hess - - Licensed under the GNU GPL version 3 or higher. -} @@ -27,6 +27,12 @@ import Logs.Transfer import Network.URI import Control.Concurrent.Async +-- youtube-dl is can follow redirects to anywhere, including potentially +-- localhost or a private address. So, it's only allowed to be used if the +-- user has allowed access to all addresses. +youtubeDlAllowed :: Annex Bool +youtubeDlAllowed = httpAddressesUnlimited + -- Runs youtube-dl in a work directory, to download a single media file -- from the url. Reutrns the path to the media file in the work directory. -- @@ -41,7 +47,10 @@ import Control.Concurrent.Async -- (Note that we can't use --output to specifiy the file to download to, -- due to ) youtubeDl :: URLString -> FilePath -> Annex (Either String (Maybe FilePath)) -youtubeDl url workdir = withUrlOptions $ youtubeDl' url workdir +youtubeDl url workdir = ifM httpAddressesUnlimited + ( withUrlOptions $ youtubeDl' url workdir + , return (Right Nothing) + ) youtubeDl' :: URLString -> FilePath -> UrlOptions -> Annex (Either String (Maybe FilePath)) youtubeDl' url workdir uo @@ -110,7 +119,13 @@ youtubeDlMaxSize workdir = ifM (Annex.getState Annex.force) -- Download a media file to a destination, youtubeDlTo :: Key -> URLString -> FilePath -> Annex Bool -youtubeDlTo key url dest = do +youtubeDlTo key url dest = ifM youtubeDlAllowed + ( youtubeDlTo' key url dest + , return False + ) + +youtubeDlTo' :: Key -> URLString -> FilePath -> Annex Bool +youtubeDlTo' key url dest = do res <- withTmpWorkDir key $ \workdir -> youtubeDl url workdir >>= \case Right (Just mediafile) -> do @@ -137,7 +152,10 @@ youtubeDlSupported url = either (const False) id <$> youtubeDlCheck url -- Check if youtube-dl can find media in an url. youtubeDlCheck :: URLString -> Annex (Either String Bool) -youtubeDlCheck = withUrlOptions . youtubeDlCheck' +youtubeDlCheck url = ifM youtubeDlAllowed + ( withUrlOptions $ youtubeDlCheck' url + , return (Right False) + ) youtubeDlCheck' :: URLString -> UrlOptions -> Annex (Either String Bool) youtubeDlCheck' url uo @@ -150,7 +168,10 @@ youtubeDlCheck' url uo -- -- (This is not always identical to the filename it uses when downloading.) youtubeDlFileName :: URLString -> Annex (Either String FilePath) -youtubeDlFileName url = withUrlOptions go +youtubeDlFileName url = ifM youtubeDlAllowed + ( withUrlOptions go + , return nomedia + ) where go uo | supportedScheme uo url = flip catchIO (pure . Left . show) $ @@ -161,7 +182,10 @@ youtubeDlFileName url = withUrlOptions go -- Does not check if the url contains htmlOnly; use when that's already -- been verified. youtubeDlFileNameHtmlOnly :: URLString -> Annex (Either String FilePath) -youtubeDlFileNameHtmlOnly = withUrlOptions . youtubeDlFileNameHtmlOnly' +youtubeDlFileNameHtmlOnly url = ifM youtubeDlAllowed + ( withUrlOptions $ youtubeDlFileNameHtmlOnly' url + , return (Left "no media in url") + ) youtubeDlFileNameHtmlOnly' :: URLString -> UrlOptions -> Annex (Either String FilePath) youtubeDlFileNameHtmlOnly' url uo diff --git a/CHANGELOG b/CHANGELOG index a16f303289..fc08d51590 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -11,10 +11,11 @@ git-annex (6.20180622) UNRELEASED; urgency=high localhost, or any private IP addresses, to prevent accidental exposure of internal data. This can be overridden with the annex.security.allowed-http-addresses setting. - * Since curl's interface does not have a way to prevent it from accessing - localhost or private IP addresses, curl defaults to not being used - for url downloads, even if annex.web-options enabled it before. - Only when annex.security.allowed-http-addresses=all will curl be used. + * Since the interfaces to curl and youtube-dl do not have a way to + prevent them from accessing localhost or private IP addresses, + they default to not being used for url downloads. + Only when annex.security.allowed-http-addresses=all will curl and + youtube-dl be used. -- Joey Hess Wed, 30 May 2018 11:49:08 -0400 diff --git a/NEWS b/NEWS index f9f42fc9e3..24672be5a1 100644 --- a/NEWS +++ b/NEWS @@ -7,14 +7,15 @@ git-annex (6.20180622) upstream; urgency=high A related security fix prevents git-annex from connecting to http servers on localhost or private networks. This can be overridden, at your own risk, using annex.security.allowed-http-addresses. + + Setting annex.web-options no longer is enough to make curl be used, + and youtube-dl is also no longer used by default. See the + documentation of annex.security.allowed-http-addresses for + details and how to enable them. The annex.web-download-command configuration has been removed, use annex.web-options instead. - Setting annex.web-options no longer is enough to make curl be used. - See the documentation of annex.security.allowed-http-addresses for - why and how to enable curl. - -- Joey Hess Fri, 15 Jun 2018 17:54:23 -0400 git-annex (6.20180309) upstream; urgency=medium diff --git a/doc/git-annex-addurl.mdwn b/doc/git-annex-addurl.mdwn index 4748073ed3..7947edc223 100644 --- a/doc/git-annex-addurl.mdwn +++ b/doc/git-annex-addurl.mdwn @@ -10,9 +10,12 @@ git annex addurl `[url ...]` Downloads each url to its own file, which is added to the annex. -When `youtube-dl` is installed, it's used to check for a video embedded in -a web page at the url, and that is added to the annex instead. - +When `youtube-dl` is installed, it can be used to check for a video +embedded in a web page at the url, and that is added to the annex instead. +(However, this is disabled by default as it can be a security risk. +See the documentation of annex.security.allowed-http-addresses +in [[git-annex]](1) for details.) + Urls to torrent files (including magnet links) will cause the content of the torrent to be downloaded, using `aria2c`. diff --git a/doc/git-annex-importfeed.mdwn b/doc/git-annex-importfeed.mdwn index 2f146fbfe0..d70b02cd90 100644 --- a/doc/git-annex-importfeed.mdwn +++ b/doc/git-annex-importfeed.mdwn @@ -13,8 +13,11 @@ content has not already been added to the repository before, so you can delete, rename, etc the resulting files and repeated runs won't duplicate them. -When `youtube-dl` is installed, it's used to download links in the feed. +When `youtube-dl` is installed, it can be used to download links in the feed. This allows importing e.g., YouTube playlists. +(However, this is disabled by default as it can be a security risk. +See the documentation of annex.security.allowed-http-addresses +in [[git-annex]](1) for details.) To make the import process add metadata to the imported files from the feed, `git config annex.genmetadata true` diff --git a/doc/git-annex.mdwn b/doc/git-annex.mdwn index 58f7e09392..c43c8012a9 100644 --- a/doc/git-annex.mdwn +++ b/doc/git-annex.mdwn @@ -1421,9 +1421,9 @@ Here are all the supported configuration settings. causing it to be downloaded into your repository transferred to other remotes, exposing its content. - Note that, since curl's interface does not allow these IP address - restrictions to be enforced, any configuration that enables use of curl - will be ignored unless annex.security.allowed-http-addresses=all. + Note that, since the interfaces of curl and youtube-dl do not allow + these IP address restrictions to be enforced, curl and youtube-dl will + never be used unless annex.security.allowed-http-addresses=all. * `annex.secure-erase-command` diff --git a/doc/tips/downloading_podcasts.mdwn b/doc/tips/downloading_podcasts.mdwn index 7416281d25..2c523606f0 100644 --- a/doc/tips/downloading_podcasts.mdwn +++ b/doc/tips/downloading_podcasts.mdwn @@ -84,6 +84,10 @@ manually. For a channel url like "https://www.youtube.com/channel/$foo", the feed is "https://www.youtube.com/feeds/videos.xml?channel_id=$foo" +Use of youtube-dl is disabled by default as it can be a security risk. +See the documentation of annex.security.allowed-http-addresses +in [[git-annex]] for details.) + ## metadata As well as storing the urls for items imported from a feed, git-annex can diff --git a/doc/tips/using_the_web_as_a_special_remote.mdwn b/doc/tips/using_the_web_as_a_special_remote.mdwn index 5e066c9183..a206879c97 100644 --- a/doc/tips/using_the_web_as_a_special_remote.mdwn +++ b/doc/tips/using_the_web_as_a_special_remote.mdwn @@ -78,6 +78,10 @@ When you have youtube-dl installed, you can just `git annex addurl http://youtube.com/foo` and it will detect that it is a video and download the video content for offline viewing. +(However, this is disabled by default as it can be a security risk. +See the documentation of annex.security.allowed-http-addresses +in [[git-annex]] for details.) + Later, in another clone of the repository, you can run `git annex get` on the file and it will also be downloaded with youtube-dl. This works even if the video host has transcoded or otherwise changed the video From 8703fdd3b75c1b249fe9143f23d1128390f391cc Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sun, 17 Jun 2018 16:13:45 -0400 Subject: [PATCH 09/41] add --- ...hole_private_data_exposure_via_addurl.mdwn | 199 ++++++++++++++++++ doc/devblog/day_499__security_hole.mdwn | 27 +++ .../day_500__security_hole_part_2.mdwn | 26 +++ .../day_501__security_hole_part_3.mdwn | 13 ++ 4 files changed, 265 insertions(+) create mode 100644 doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn create mode 100644 doc/devblog/day_499__security_hole.mdwn create mode 100644 doc/devblog/day_500__security_hole_part_2.mdwn create mode 100644 doc/devblog/day_501__security_hole_part_3.mdwn diff --git a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn new file mode 100644 index 0000000000..42ba0dd9ae --- /dev/null +++ b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn @@ -0,0 +1,199 @@ +This is a security hole that allows exposure of +private data in files located outside the git-annex repository. + +The attacker needs to have control over one of the remotes of the git-annex +repository. For example, they may provide a public git-annex repository +that the victim clones. Or the victim may have paired repositories with +them. Or, equivilantly, the attacker could have read access to the victim's +git-annex repository (eg on a server somewhere), and some channel to get +commits into it (eg a pull requests). + +The attacker does `git-annex addurl --relaxed file:///etc/passwd` and commits +this to the repository in some out of the way place. Then they wait for the +victim to pull the change. + +The easiest exploit is when the victim is running the assistant, or is +periodically doing `git annex sync --content`. The victim may also perform +the equivilant actions manually, not noticing they're operating on the +file. + +In either case, git-annex gets the content of the annexed file, following +the file:// url. Then git-annex transfers the content back to the +attacker's repository. + +Note that the victim can always detect this attack after the fact, since +even if the attacker deletes the file, the git history will still +contain it. + +It may also be possible to exploit scp:// sftp:// smb:// etc urls to get at +files on other computers that the user has access to as well as localhost. +I was not able to get curl to download from scp:// or sftp:// on debian +(unstable) but there may be configurations that allow it. + +If the url is attached to a key using a cryptographic hash, then the +attacker would need to already know at least the hash of the content +to exploit this. + +Sending that content back to them could be considered not a security +hole. Except, I can guess what content some file on your system might have, +and use this attack as an oracle to determine if I guessed right, and work +toward penetrating your system using that information. + +So, best to not treat addurl with a hash any differently than +--relaxed and --fast when addressing this hole. + +---- + +The fix must involve locking down the set of allowed in url schemes. +Better to have a whitelist than a blacklist. http and https seems like the +right default. + +Some users do rely on file:// urls, and this is fine in some use cases, +eg when you're not merging changes from anyone else. + +So this probably needs to be a git config of allowed url schemes, +with an appropriatly scary name, like `annex.security.allowed-url-schemes`. + +Redirects from one url scheme to another could be usd to bypass such a +whitelist. curl's `--proto` also affects redirects. http-conduit +is limited to only http and will probably remain that way. + +> done in [[!commit 28720c795ff57a55b48e56d15f9b6bcb977f48d9]] --[[Joey]] + +---- + +The same kind of attack can be used to see the content of +localhost urls and other non-globally-available urls. + +Redirects and DNS rebinding attacks mean that checking the IP address +of the hostname in the url is not good enough. It needs to check the IP +address that is actually connected to, for each http connection made, +including redirects. + +There will need to be a config to relax checks, like +with an appropriatly scary name, like +`annex.security.allowed-http-addresses`. Users will want to enable +support for specific subnets, or perhaps allow all addresses. + +When git-annex is configured to use curl, there seems to be no way +to prevent curl from following urls that redirect to localhost, other than +disabling redirections. And unless git-annex also pre-resolves the IP +address and rewrites it into the url passed to curl, DNS rebinding attacks +would still be possible. Also, one of the remaining reasons people enable +curl is to use a netrc file with passwords, and the content of +urls on those sites could also be exposed by this attack. So, it seems curl +should not be enabled by default and should have a big security warning if +it's supported at all. Probably ought to only enable it +when `annex.security.allowed-http-addresses=all` + +http-client does not have hooks that can be used to find out what IP +address it's connecting to in a secure enough way. +See + +Seems that building my own http Manager is the only way to go. By building +my own, I can do the IP address checks inside it when it's setting up +connections. And, the same manager can be passed to the S3 and WebDav libraries. +(The url scheme checks could also be moved into that Manager, to prevent +S3 redirect to file: url scenarios..) + +> restricted http manager done and used in +> [[!commit b54b2cdc0ef1373fc200c0d28fded3c04fd57212]]; +> curl also disabled by default + +http proxies are another problem. They could be on the local network, +or even on localhost, and http-client does not provide a way to force +a http proxy to connect to an particular IP address (is that even possible?) +May have to disable use of http proxies unless +`annex.security.allowed-http-addresses=all` +Or better, find what http proxy is enabled and prevent using it if it's on +an IP not allowed there. + +> TODO + +---- + +The external special remote interface is another way to exploit this. +Since it bypasses git-annex's usual url download code, whatever fixes are +put in place there won't help external special remotes. + +External special remotes that use GETURLS, typically in conjunction with +CLAIMURL and CHECKURL, and then themselves download the content of urls +in response to a TRANSFER RETRIEVE will have the same problems as +git-annex's url downloading. + +> Checked all known external special remotes +> to see if they used GETURLS; only one that did is ipfs, which doesn't use +> http so is ok. + +> TODO document this security issue in special remote protocol page + +An external special remote might also make a simple http request to a +key/value API to download a key, and follow a redirect to file:/// +or http://localhost/. + +If the key uses a cryptographic hash, git-annex verifies the content. +But, the attacker could have committed a key that doesn't +use a hash. Also, the attacker could use the hash check as an oracle. + +What if an external special remote uses https to a hardcoded server (either +fully hardcoded, or configured by the user when they enable the remote), +and uses a http library that only supports https and http (not file:///). +Is that good enough? An attacker would need to either compromise the server +to redirect to a local private webserver, or MITM, and https should prevent +the MITM. + +This seems to require auditing of all external special remotes. +git-annex could add a new command to the external protocol, that asks +the special remote if it's been audited, and refuse to use ones that have +not. + +I have emailed all relevant external special remote authors a heads-up and +some questions. + +> TODO + +---- + +Built-in special remotes that use protocols on top of http, eg S3 and WebDAV, +don't use Utility.Url, could also be exploited, and will need to be fixed +separately. + +> not affected for url schemes, because http-client only supports http, +> not file:/ + +> done for localhost/lan in [[!commit b54b2cdc0ef1373fc200c0d28fded3c04fd57212]] + +youtube-dl + +> already disabled file:/. Added a scheme check, but it won't block +> redirects to other schemes. But youtube-dl goes off and does its own thing with other +> protocols anyway, so that's fine. +> +> The youtube-dl generic extractor will download media files (including +> videos and photos) if passed an direct url to them. It does not seem to +> extract video etc from tags on html pages. + +> git-annex first checks if a web page +> is html before pointing youtube-dl at it, to avoid using it to download +> direct urls to media files. But that would not prevent a DNS rebinding +> attack which made git-annex see the html page, and youtube-dl then see +> a media file on localhost. +> +> Also, the current code in htmlOnly +> runs youtube-dl if the html page download test fails to download +> anything. +> +> Also, in the course of a download, youtube-dl could chain to other urls, +> depending on how its extractor works. Those urls could redirect to +> a localhost/lan web server. +> +> So, youtube-dl needs to be disabled unless http address security checks +> are turned off. +> +> > done in [[!commit e62c4543c31a61186ebf2e4e0412df59fc8630c8]] + +glacier + +> This special remote uses glacier-cli, which will need to be audited. +> Emailed basak. +> TODO diff --git a/doc/devblog/day_499__security_hole.mdwn b/doc/devblog/day_499__security_hole.mdwn new file mode 100644 index 0000000000..9f0d532d6d --- /dev/null +++ b/doc/devblog/day_499__security_hole.mdwn @@ -0,0 +1,27 @@ +I'm writing this on a private branch, it won't be posted until a week from +now when the security hole is disclosed. + +Security is not compositional. You can have one good feature, and add +another good feature, and the result is not two good features, but a new +security hole. In this case +[[bugs/security_hole_private_data_exposure_via_addurl]]. And it can be hard +to spot this kind of security hole, but then once it's known it +seems blindly obvious. + +It came to me last night and by this morning I had decided the potential +impact was large enough to do a coordinated disclosure. Spent the first +half of the day thinking through ways to fix it that don't involve writing +my own http library. Then started getting in touch with all the +distributions' security teams. And then coded up a fairly complete fix for +the worst part of the security hole, although a secondary part is going to +need considerably more work. + +It looks like the external special remotes are going to need at least some +security review too, and I'm still thinking that part of the problem over. + +Exhausted. + +Today's work was sponsored by Trenton Cronholm +[on Patreon](https://patreon.com/joeyh). + +[[!meta date="Jun 15 2018 7:00 pm"]] diff --git a/doc/devblog/day_500__security_hole_part_2.mdwn b/doc/devblog/day_500__security_hole_part_2.mdwn new file mode 100644 index 0000000000..3d485ee73c --- /dev/null +++ b/doc/devblog/day_500__security_hole_part_2.mdwn @@ -0,0 +1,26 @@ +Most of the day was spent staring at the http-client source code and trying +to find a way to add the IP address checks to it that I need to fully close +the security hole. + +In the end, I did find a way, with the duplication of a couple dozen lines +of code from http-client. It will let the security fix be used with +libraries like aws and DAV that build on top of http-client, too. + +While the code is in git-annex for now, it's fully disconnected and +would also be useful if a web browser were implemented in Haskell, +to implement same-origin restrictions while avoiding DNS rebinding attacks. + +Looks like http proxies and curl will need to be disabled by default, +since this fix can't support either of them securely. I wonder how web +browsers deal with http proxies, DNS rebinding attacks and same-origin? +I can't think of a secure way. + +Next I need a function that checks if an IP address is a link-local address +or a private network address. For both ipv4 and ipv6. Could not find +anything handy on hackage, so I'm gonna have to stare at some RFCs. Perhaps +this evening, for now, it's time to swim in the river. + +Today's work was sponsored by Jake Vosloo +[on Patreon](https://patreon.com/joeyh) + +[[!meta date="June 16 2018 4:00 pm"]] diff --git a/doc/devblog/day_501__security_hole_part_3.mdwn b/doc/devblog/day_501__security_hole_part_3.mdwn new file mode 100644 index 0000000000..6187f7d534 --- /dev/null +++ b/doc/devblog/day_501__security_hole_part_3.mdwn @@ -0,0 +1,13 @@ +Got the IP address restrictions for http implemented. (Except for http +proxies.) + +Unforunately as part of this, had to make youtube-dl and curl not be used +by default. The annex.security.allowed-http-addresses config has to be +opened up by the user in order to use those external commands, since they +can follow arbitrary redirects. + +Also thought some more about how external special remotes might be +affected, and sent their authors' a heads-up. + +[[!meta date="June 17 2018 4:00 pm"]] + From cc08135e659d3ca9ea157246433d8aa90de3baf7 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 18 Jun 2018 13:32:20 -0400 Subject: [PATCH 10/41] prevent using local http proxies per annex.security.allowed-http-addresses A local http proxy would bypass the security configuration. So, the security configuration has to be applied when choosing whether to use the proxy. While http rebinding attacks against the dns lookup of the proxy IP address seem very unlikely, this implementation does prevent them, since it resolves the IP address once, checks it, and then reconfigures http-client's proxy using the resolved address. This commit was sponsored by Ole-Morten Duesund on Patreon. --- Annex/Url.hs | 7 +- CHANGELOG | 2 + NEWS | 4 +- Utility/HttpManagerRestricted.hs | 137 ++++++++++++++++++++++++------- 4 files changed, 117 insertions(+), 33 deletions(-) diff --git a/Annex/Url.hs b/Annex/Url.hs index c91c54dac3..53a4bfbc34 100644 --- a/Annex/Url.hs +++ b/Annex/Url.hs @@ -81,8 +81,13 @@ getUrlOptions = Annex.getState Annex.urloptions >>= \case then Nothing else Just (addrConnectionRestricted addr) } - manager <- liftIO $ U.newManager $ + (settings, pr) <- liftIO $ restrictManagerSettings r U.managerSettings + case pr of + Nothing -> return () + Just ProxyRestricted -> toplevelWarning True + "http proxy settings not used due to annex.security.allowed-http-addresses configuration" + manager <- liftIO $ U.newManager settings return (U.DownloadWithConduit, manager) httpAddressesUnlimited :: Annex Bool diff --git a/CHANGELOG b/CHANGELOG index fc08d51590..a212011dd0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -11,6 +11,8 @@ git-annex (6.20180622) UNRELEASED; urgency=high localhost, or any private IP addresses, to prevent accidental exposure of internal data. This can be overridden with the annex.security.allowed-http-addresses setting. + * Local http proxies will not be used unless allowed by the + annex.security.allowed-http-addresses setting. * Since the interfaces to curl and youtube-dl do not have a way to prevent them from accessing localhost or private IP addresses, they default to not being used for url downloads. diff --git a/NEWS b/NEWS index 24672be5a1..f09009dbb0 100644 --- a/NEWS +++ b/NEWS @@ -5,8 +5,8 @@ git-annex (6.20180622) upstream; urgency=high using annex.security.allowed-url-schemes. A related security fix prevents git-annex from connecting to http - servers on localhost or private networks. This can be overridden, - at your own risk, using annex.security.allowed-http-addresses. + servers (and proxies) on localhost or private networks. This can + be overridden, at your own risk, using annex.security.allowed-http-addresses. Setting annex.web-options no longer is enough to make curl be used, and youtube-dl is also no longer used by default. See the diff --git a/Utility/HttpManagerRestricted.hs b/Utility/HttpManagerRestricted.hs index 2b1e241cc4..c264ce2ef8 100644 --- a/Utility/HttpManagerRestricted.hs +++ b/Utility/HttpManagerRestricted.hs @@ -7,25 +7,27 @@ - License: MIT -} -{-# LANGUAGE ScopedTypeVariables, DeriveDataTypeable #-} +{-# LANGUAGE ScopedTypeVariables, DeriveDataTypeable, LambdaCase, PatternGuards #-} module Utility.HttpManagerRestricted ( restrictManagerSettings, Restriction(..), ConnectionRestricted(..), addrConnectionRestricted, + ProxyRestricted(..), ) where import Network.HTTP.Client -import Network.HTTP.Client.Internal (ManagerSettings(..), Connection) -import Network.HTTP.Client.TLS +import Network.HTTP.Client.Internal + (ManagerSettings(..), Connection, runProxyOverride) import Network.Socket import Network.BSD (getProtocolNumber) import Control.Exception import qualified Network.Connection as NC -import Data.ByteString (ByteString) +import qualified Data.ByteString.UTF8 as BU import Data.Default import Data.Typeable +import Control.Applicative data Restriction = Restriction { addressRestriction :: AddrInfo -> Maybe ConnectionRestricted @@ -39,31 +41,95 @@ instance Exception ConnectionRestricted addrConnectionRestricted :: AddrInfo -> ConnectionRestricted addrConnectionRestricted addr = ConnectionRestricted $ unwords [ "Configuration does not allow accessing address" - , case addrAddress addr of - a@(SockAddrInet _ _) -> - takeWhile (/= ':') $ show a - a@(SockAddrInet6 _ _ _ _) -> - takeWhile (/= ']') $ drop 1 $ show a + , showSockAddress (addrAddress addr) ] --- | Adjusts a ManagerSettings to check a Restriction. +data ProxyRestricted = ProxyRestricted + deriving (Show) + +-- | Adjusts a ManagerSettings to enforce a Restriction. -- --- Note that connections to http proxies are not checked. --- Use `managerSetProxy noProxy` to prevent connections through http --- proxies. -restrictManagerSettings :: Restriction -> ManagerSettings -> ManagerSettings -restrictManagerSettings cfg base = base +-- This includes checking the http proxy against the Restriction. +-- If access to it is blocked, the ManagerSettings will be made to +-- not use the proxy. +restrictManagerSettings + :: Restriction + -> ManagerSettings + -> IO (ManagerSettings, Maybe ProxyRestricted) +restrictManagerSettings cfg base = restrictProxy cfg $ base { managerRawConnection = restrictedRawConnection cfg , managerTlsConnection = restrictedTlsConnection cfg - , managerWrapException = \req -> - let wrapper se - | Just (_ :: ConnectionRestricted) <- fromException se = - toException $ HttpExceptionRequest req $ - InternalException se - | otherwise = se - in handle $ throwIO . wrapper + , managerWrapException = wrapOurExceptions } +restrictProxy + :: Restriction + -> ManagerSettings + -> IO (ManagerSettings, Maybe ProxyRestricted) +restrictProxy cfg base = do + http_proxy_addr <- getproxyaddr False + https_proxy_addr <- getproxyaddr True + let (http_proxy, http_r) = mkproxy http_proxy_addr + let (https_proxy, https_r) = mkproxy https_proxy_addr + let ms = managerSetInsecureProxy http_proxy $ + managerSetSecureProxy https_proxy base + return (ms, http_r <|> https_r) + where + -- This does not use localhost because http-client may choose + -- not to use the proxy for localhost. + testnetip = "198.51.100.1" + dummyreq https = parseRequest_ $ + "http" ++ (if https then "s" else "") ++ "://" ++ testnetip + + getproxyaddr https = extractproxy >>= \case + Nothing -> return Nothing + Just p -> do + proto <- getProtocolNumber "tcp" + let serv = show (proxyPort p) + let hints = defaultHints + { addrFlags = [AI_ADDRCONFIG] + , addrProtocol = proto + , addrSocketType = Stream + } + let h = BU.toString $ proxyHost p + getAddrInfo (Just hints) (Just h) (Just serv) >>= \case + [] -> return Nothing + (addr:_) -> return $ Just addr + where + -- These contortions are necessary until this issue + -- is fixed: + -- https://github.com/snoyberg/http-client/issues/355 + extractproxy = do + let po = if https + then managerProxySecure base + else managerProxyInsecure base + f <- runProxyOverride po https + return $ proxy $ f $ dummyreq https + + mkproxy Nothing = (noProxy, Nothing) + mkproxy (Just proxyaddr) = case addressRestriction cfg proxyaddr of + Nothing -> (addrtoproxy (addrAddress proxyaddr), Nothing) + Just _ -> (noProxy, Just ProxyRestricted) + + addrtoproxy addr = case addr of + SockAddrInet pn _ -> mk pn + SockAddrInet6 pn _ _ _ -> mk pn + _ -> noProxy + where + mk pn = useProxy Network.HTTP.Client.Proxy + { proxyHost = BU.fromString (showSockAddress addr) + , proxyPort = fromIntegral pn + } + +wrapOurExceptions :: Request -> IO a -> IO a +wrapOurExceptions req = + let wrapper se + | Just (_ :: ConnectionRestricted) <- fromException se = + toException $ HttpExceptionRequest req $ + InternalException se + | otherwise = se + in handle $ throwIO . wrapper + restrictedRawConnection :: Restriction -> IO (Maybe HostAddress -> String -> Int -> IO Connection) restrictedRawConnection cfg = getConnection cfg Nothing @@ -76,6 +142,8 @@ restrictedTlsConnection cfg = getConnection cfg $ -- less secure, this is not a big problem. Just def + + -- Based on Network.HTTP.Client.TLS.getTlsConnection. -- -- Checks the Restriction @@ -84,26 +152,26 @@ restrictedTlsConnection cfg = getConnection cfg $ getConnection :: Restriction -> Maybe NC.TLSSettings -> IO (Maybe HostAddress -> String -> Int -> IO Connection) getConnection cfg tls = do context <- NC.initConnectionContext - return $ \_ha host port -> bracketOnError - (go context host port) + return $ \_ha h p -> bracketOnError + (go context h p) NC.connectionClose convertConnection where - go context host port = do + go context h p = do let connparams = NC.ConnectionParams - { NC.connectionHostname = host - , NC.connectionPort = fromIntegral port + { NC.connectionHostname = h + , NC.connectionPort = fromIntegral p , NC.connectionUseSecure = tls , NC.connectionUseSocks = Nothing -- unsupprted } proto <- getProtocolNumber "tcp" - let serv = show port + let serv = show p let hints = defaultHints { addrFlags = [AI_ADDRCONFIG] , addrProtocol = proto , addrSocketType = Stream } - addrs <- getAddrInfo (Just hints) (Just host) (Just serv) + addrs <- getAddrInfo (Just hints) (Just h) (Just serv) bracketOnError (firstSuccessful $ map tryToConnect addrs) close @@ -115,7 +183,7 @@ getConnection cfg tls = do close (\sock -> connect sock (addrAddress addr) >> return sock) Just r -> throwIO r - firstSuccessful [] = throwIO $ NC.HostNotResolved host + firstSuccessful [] = throwIO $ NC.HostNotResolved h firstSuccessful (a:as) = a `catch` \(e ::IOException) -> case as of [] -> throwIO e @@ -130,3 +198,12 @@ convertConnection conn = makeConnection -- on the socket. But when this is called the socket might be -- already closed, and we get a @ResourceVanished@. (NC.connectionClose conn `Control.Exception.catch` \(_ :: IOException) -> return ()) + +-- For ipv4 and ipv6, the string will contain only the IP address, +-- omitting the port that the Show instance includes. +showSockAddress :: SockAddr -> String +showSockAddress a@(SockAddrInet _ _) = + takeWhile (/= ':') $ show a +showSockAddress a@(SockAddrInet6 _ _ _ _) = + takeWhile (/= ']') $ drop 1 $ show a +showSockAddress a = show a From 71d39caf5cfa1793134827691c1c687eb281a617 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 18 Jun 2018 14:19:58 -0400 Subject: [PATCH 11/41] add security page with current and past security holes --- ...hole_private_data_exposure_via_addurl.mdwn | 18 +++++----- doc/news.mdwn | 2 +- doc/security.mdwn | 4 +++ doc/security/CVE-2014-6274.mdwn | 10 ++++++ doc/security/CVE-2017-12976.mdwn | 10 ++++++ ...exposure_to_encrypted_special_remotes.mdwn | 12 +++++++ doc/security/private_data_exposure.mdwn | 36 +++++++++++++++++++ 7 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 doc/security.mdwn create mode 100644 doc/security/CVE-2014-6274.mdwn create mode 100644 doc/security/CVE-2017-12976.mdwn create mode 100644 doc/security/checksum_exposure_to_encrypted_special_remotes.mdwn create mode 100644 doc/security/private_data_exposure.mdwn diff --git a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn index 42ba0dd9ae..54d4ac60dc 100644 --- a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn +++ b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn @@ -21,10 +21,6 @@ In either case, git-annex gets the content of the annexed file, following the file:// url. Then git-annex transfers the content back to the attacker's repository. -Note that the victim can always detect this attack after the fact, since -even if the attacker deletes the file, the git history will still -contain it. - It may also be possible to exploit scp:// sftp:// smb:// etc urls to get at files on other computers that the user has access to as well as localhost. I was not able to get curl to download from scp:// or sftp:// on debian @@ -108,7 +104,7 @@ May have to disable use of http proxies unless Or better, find what http proxy is enabled and prevent using it if it's on an IP not allowed there. -> TODO +> done in [[!commit cc08135e659d3ca9ea157246433d8aa90de3baf7]] ---- @@ -122,8 +118,8 @@ in response to a TRANSFER RETRIEVE will have the same problems as git-annex's url downloading. > Checked all known external special remotes -> to see if they used GETURLS; only one that did is ipfs, which doesn't use -> http so is ok. +> to see if they used GETURLS. ipfs did, but not in an exploitable way. +> datalad does; looped in its developers to asess. Rest appear not to. > TODO document this security issue in special remote protocol page @@ -195,5 +191,9 @@ youtube-dl glacier > This special remote uses glacier-cli, which will need to be audited. -> Emailed basak. -> TODO +> Emailed Robie Basak about it, and he looked into the http libraries +> used by glacier-cli and boto. It appears that they do not support +> file:///. It also appears that the libraries do not handle redirects +> themselves, and that boto does not handle http redirects. glacier-cli +> uses https. Combining all this, it seems that glacier-cli is not +> vulnerable to this class of attacks. diff --git a/doc/news.mdwn b/doc/news.mdwn index 95114cdcf5..0cb92355d8 100644 --- a/doc/news.mdwn +++ b/doc/news.mdwn @@ -1,7 +1,7 @@ [[!if test="news/*" then=""" This is where announcements of new releases, features, and other news is posted. git-annex users are recommended to subscribe to this page's RSS -feed. +feed. Also, see [[security]] for security announcements. [[!inline pages="./news/* and !./news/*/* and !*/Discussion" rootpage="news" show="30"]] diff --git a/doc/security.mdwn b/doc/security.mdwn new file mode 100644 index 0000000000..c88b2106b1 --- /dev/null +++ b/doc/security.mdwn @@ -0,0 +1,4 @@ +Information about all known security holes in git-annex, and their fixes. +Subscribe to the RSS feed to be kept up to date. + +[[!inline pages="./security/* and !./security/*/* and !*/Discussion" rootpage="security" show="0"]] diff --git a/doc/security/CVE-2014-6274.mdwn b/doc/security/CVE-2014-6274.mdwn new file mode 100644 index 0000000000..8e9f4ae5aa --- /dev/null +++ b/doc/security/CVE-2014-6274.mdwn @@ -0,0 +1,10 @@ +CVE-2014-6274: Security fix for S3 and glacier when using embedcreds=yes with +encryption=pubkey or encryption=hybrid. + +The creds embedded in the git repo were *not* encrypted. +git-annex enableremote will warn when used on a remote that has +this problem. For details, see [[upgrades/insecure_embedded_creds]]. + +Fixed in git-annex 5.20140919. + +[[!meta date="Fri, 19 Sep 2014 12:53:42 -0400"]] diff --git a/doc/security/CVE-2017-12976.mdwn b/doc/security/CVE-2017-12976.mdwn new file mode 100644 index 0000000000..bbc8961b1e --- /dev/null +++ b/doc/security/CVE-2017-12976.mdwn @@ -0,0 +1,10 @@ +CVE-2017-12976: A hostname starting with a dash would get passed to ssh and be treated as +an option. This could be used by an attacker who provides a crafted +repository url to cause the victim to execute arbitrary code via +`-oProxyCommand`. + +Fixed in git-annex 6.20170818 + +This is related to a git security hole, [CVE-2017-1000117](https://marc.info/?l=git&m=150238802328673&w=2). + +[[!meta date="Fri, 18 Aug 2017 11:19:06 -0400"]] diff --git a/doc/security/checksum_exposure_to_encrypted_special_remotes.mdwn b/doc/security/checksum_exposure_to_encrypted_special_remotes.mdwn new file mode 100644 index 0000000000..4301e1fda9 --- /dev/null +++ b/doc/security/checksum_exposure_to_encrypted_special_remotes.mdwn @@ -0,0 +1,12 @@ +A bug exposed the checksum of annexed files to encrypted +special remotes, which are not supposed to have access to the checksum of +the un-encrypted file. This only occurred when resuming uploads to the +encrypted special remote, so it is considered a low-severity security hole. + +For details, see [[!commit b890f3a53d936b5e40aa9acc5876cb98f18b9657]] + +No CVE was assigned for this issue. + +Fixed in git-annex 6.20160419 + +[[!meta date="Thu, 28 Apr 2016 09:31:14 -0400"]] diff --git a/doc/security/private_data_exposure.mdwn b/doc/security/private_data_exposure.mdwn new file mode 100644 index 0000000000..ffcabffed5 --- /dev/null +++ b/doc/security/private_data_exposure.mdwn @@ -0,0 +1,36 @@ +Some uses of git-annex were vulnerable to a private data exposure and +exfiltration attack. It could expose the content of files located +outside the git-annex repository, or content from a private +web server on localhost or the LAN. + +This was fixed in git-annex 6.20180622. + +## details + +The attacker needed to have control over one of the remotes of the git-annex +repository. For example, they may provide a public git-annex repository that +the victim clones. Or the victim may have paired repositories with them. Or, +equivilantly, the attacker could have read access to the victim's git-annex +repository (eg on a server somewhere), and some channel to get commits into it +(eg a pull requests). + +The attacker does `git-annex addurl --relaxed file:///etc/passwd` and commits +this to the repository in some out of the way place. Then they wait for the +victim to pull the change. (As well as `file:///` urls, the attacker can +use urls to private web servers. The url can also be one that the attacker +controls, that redirects to such urls.) + +The easiest exploit is when the victim is running the git-annex assistant, or +is periodically doing `git annex sync --content`. The victim may also perform +the equivilant actions manually, not noticing they're operating on the file. + +In either case, git-annex gets the content of the annexed file, following +the attacker-provider url to private data. Then git-annex transfers the content +back to the attacker's repository. + +This attack was fixed by making git-annex refuse to follow `file:///` urls +and urls pointing to private/local IP addresses by default. Two new +configuration settings, annex.security.allowed-url-schemes and +annex.security.allowed-http-addresses, can relax this security policy, +and are intended for cases where the git-annex repository is kept +private and so the attack does not apply. From c93b6c1e08e7733a0efae112f18bcad977fe1236 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 18 Jun 2018 14:25:55 -0400 Subject: [PATCH 12/41] devblog --- doc/devblog/day_502__security_hole_part_4.mdwn | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 doc/devblog/day_502__security_hole_part_4.mdwn diff --git a/doc/devblog/day_502__security_hole_part_4.mdwn b/doc/devblog/day_502__security_hole_part_4.mdwn new file mode 100644 index 0000000000..48c38de5fe --- /dev/null +++ b/doc/devblog/day_502__security_hole_part_4.mdwn @@ -0,0 +1,16 @@ +Spent several hours dealing with the problem of http proxies, which +bypassed the IP address checks added to prevent the security hole. +Eventually got it filtering out http proxies located on private IP +addresses. + +Other than the question of what to do about external special remotes +that may be vulerable to related problems, it looks like the security +hole is all closed off in git-annex now. + +Added a new page [[security]] with details of this and past security holes +in git-annex. + +Several people I reached out to for help with special remotes have gotten +back to me, and we're discussing how the security hole may affect them and +what to do. Thanks especially to Robie Basak and Daniel Dent for their +work on security analysis. From 3c0a538335a71a2ecfa7d76f0a87da9e24b6804c Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 18 Jun 2018 15:36:12 -0400 Subject: [PATCH 13/41] allow ftp urls by default They're no worse than http certianly. And, the backport of these security fixes has to deal with wget, which supports http https and ftp and has no way to turn off individual schemes, so this will make that easier. --- CHANGELOG | 2 +- NEWS | 2 +- Types/GitConfig.hs | 2 +- Utility/Url.hs | 2 +- doc/git-annex.mdwn | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index a212011dd0..2cc8490190 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,7 +1,7 @@ git-annex (6.20180622) UNRELEASED; urgency=high * Added annex.security.allowed-url-schemes setting, which defaults - to only allowing http and https URLs. Note especially that file:/ + to only allowing http, https, and ftp URLs. Note especially that file:/ is no longer enabled by default. This is a security fix. * Removed annex.web-download-command, since its interface does not allow supporting annex.security.allowed-url-schemes across redirects. diff --git a/NEWS b/NEWS index f09009dbb0..2dcc72e52b 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,6 @@ git-annex (6.20180622) upstream; urgency=high - A security fix has changed git-annex to only support http and https + A security fix has changed git-annex to only support http, https, and ftp URL schemes by default. You can enable other URL schemes, at your own risk, using annex.security.allowed-url-schemes. diff --git a/Types/GitConfig.hs b/Types/GitConfig.hs index 98c8c6e83c..7d9ccbff1f 100644 --- a/Types/GitConfig.hs +++ b/Types/GitConfig.hs @@ -162,7 +162,7 @@ extractGitConfig r = GitConfig , annexRetryDelay = Seconds <$> getmayberead (annex "retrydelay") , annexAllowedUrlSchemes = S.fromList $ map mkScheme $ - maybe ["http", "https"] words $ + maybe ["http", "https", "ftp"] words $ getmaybe (annex "security.allowed-url-schemes") , annexAllowedHttpAddresses = fromMaybe "" $ getmaybe (annex "security.allowed-http-addresses") diff --git a/Utility/Url.hs b/Utility/Url.hs index 5664c47124..3301a4b0ef 100644 --- a/Utility/Url.hs +++ b/Utility/Url.hs @@ -100,7 +100,7 @@ defUrlOptions = UrlOptions <*> pure DownloadWithConduit <*> pure id <*> newManager managerSettings - <*> pure (S.fromList $ map mkScheme ["http", "https"]) + <*> pure (S.fromList $ map mkScheme ["http", "https", "ftp"]) mkUrlOptions :: Maybe UserAgent -> Headers -> UrlDownloader -> Manager -> S.Set Scheme -> UrlOptions mkUrlOptions defuseragent reqheaders urldownloader manager = diff --git a/doc/git-annex.mdwn b/doc/git-annex.mdwn index c43c8012a9..e3386f3939 100644 --- a/doc/git-annex.mdwn +++ b/doc/git-annex.mdwn @@ -1394,7 +1394,7 @@ Here are all the supported configuration settings. * `annex.security.allowed-url-schemes` List of URL schemes that git-annex is allowed to download content from. - The default is "http https". + The default is "http https ftp". Think very carefully before changing this; there are security implications. For example, if it's changed to allow "file" URLs, then From e00b3ab3d53085ff8ea8e89040c9188b9ab0fa2f Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 18 Jun 2018 15:57:13 -0400 Subject: [PATCH 14/41] doc typo --- doc/git-annex.mdwn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/git-annex.mdwn b/doc/git-annex.mdwn index e3386f3939..5c84249763 100644 --- a/doc/git-annex.mdwn +++ b/doc/git-annex.mdwn @@ -1418,7 +1418,7 @@ Here are all the supported configuration settings. Think very carefully before changing this; there are security implications. Anyone who can get a commit into your git-annex repository could `git annex addurl` an url on a private http server, possibly - causing it to be downloaded into your repository transferred to + causing it to be downloaded into your repository and transferred to other remotes, exposing its content. Note that, since the interfaces of curl and youtube-dl do not allow From c81b879d39a34e81a9de9037ecf0ce06cdac6f18 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 18 Jun 2018 17:56:18 -0400 Subject: [PATCH 15/41] got a CVE number --- CHANGELOG | 1 + .../security_hole_private_data_exposure_via_addurl.mdwn | 2 ++ .../{private_data_exposure.mdwn => CVE-2018-10857.mdwn} | 8 ++++---- 3 files changed, 7 insertions(+), 4 deletions(-) rename doc/security/{private_data_exposure.mdwn => CVE-2018-10857.mdwn} (86%) diff --git a/CHANGELOG b/CHANGELOG index 2cc8490190..1042448bc9 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,5 +1,6 @@ git-annex (6.20180622) UNRELEASED; urgency=high + * Security fix release for CVE-2018-10857. * Added annex.security.allowed-url-schemes setting, which defaults to only allowing http, https, and ftp URLs. Note especially that file:/ is no longer enabled by default. This is a security fix. diff --git a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn index 54d4ac60dc..b27eb64d4e 100644 --- a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn +++ b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn @@ -1,3 +1,5 @@ +CVE-2018-10857 + This is a security hole that allows exposure of private data in files located outside the git-annex repository. diff --git a/doc/security/private_data_exposure.mdwn b/doc/security/CVE-2018-10857.mdwn similarity index 86% rename from doc/security/private_data_exposure.mdwn rename to doc/security/CVE-2018-10857.mdwn index ffcabffed5..bad9edd5fa 100644 --- a/doc/security/private_data_exposure.mdwn +++ b/doc/security/CVE-2018-10857.mdwn @@ -1,7 +1,7 @@ -Some uses of git-annex were vulnerable to a private data exposure and -exfiltration attack. It could expose the content of files located -outside the git-annex repository, or content from a private -web server on localhost or the LAN. +CVE-2018-10857: Some uses of git-annex were vulnerable to a private data +exposure and exfiltration attack. It could expose the content of files +located outside the git-annex repository, or content from a private web +server on localhost or the LAN. This was fixed in git-annex 6.20180622. From daac67c9b1212cfd2262ab499ed55676afb6cdf3 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 18 Jun 2018 18:01:33 -0400 Subject: [PATCH 16/41] update --- doc/devblog/day_502__security_hole_part_4.mdwn | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/devblog/day_502__security_hole_part_4.mdwn b/doc/devblog/day_502__security_hole_part_4.mdwn index 48c38de5fe..43b66300b3 100644 --- a/doc/devblog/day_502__security_hole_part_4.mdwn +++ b/doc/devblog/day_502__security_hole_part_4.mdwn @@ -14,3 +14,9 @@ Several people I reached out to for help with special remotes have gotten back to me, and we're discussing how the security hole may affect them and what to do. Thanks especially to Robie Basak and Daniel Dent for their work on security analysis. + +Also prepared a minimal backport of the security fixes for the git-annex in +Debian stable, which will probably be more palatable to their security team +than the full 2000+ lines of patches I've developed so far. +The minimal fix is secure, but suboptimal; it prevents even safe urls from +being downloaded from the web special remote by default. From fc79f68404c49ddd62bb92a5cba79a185ed93595 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 19 Jun 2018 11:25:10 -0400 Subject: [PATCH 17/41] support building on debian stable Specifically, http-client-0.4.31 This commit was supported by the NSF-funded DataLad project. --- Utility/HttpManagerRestricted.hs | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/Utility/HttpManagerRestricted.hs b/Utility/HttpManagerRestricted.hs index c264ce2ef8..9d39464c7a 100644 --- a/Utility/HttpManagerRestricted.hs +++ b/Utility/HttpManagerRestricted.hs @@ -8,6 +8,7 @@ -} {-# LANGUAGE ScopedTypeVariables, DeriveDataTypeable, LambdaCase, PatternGuards #-} +{-# LANGUAGE CPP #-} module Utility.HttpManagerRestricted ( restrictManagerSettings, @@ -19,7 +20,7 @@ module Utility.HttpManagerRestricted ( import Network.HTTP.Client import Network.HTTP.Client.Internal - (ManagerSettings(..), Connection, runProxyOverride) + (ManagerSettings(..), Connection, runProxyOverride, makeConnection) import Network.Socket import Network.BSD (getProtocolNumber) import Control.Exception @@ -59,7 +60,11 @@ restrictManagerSettings restrictManagerSettings cfg base = restrictProxy cfg $ base { managerRawConnection = restrictedRawConnection cfg , managerTlsConnection = restrictedTlsConnection cfg +#if MIN_VERSION_http_client(0,5,0) , managerWrapException = wrapOurExceptions +#else + , managerWrapIOException = wrapOurExceptions +#endif } restrictProxy @@ -121,6 +126,7 @@ restrictProxy cfg base = do , proxyPort = fromIntegral pn } +#if MIN_VERSION_http_client(0,5,0) wrapOurExceptions :: Request -> IO a -> IO a wrapOurExceptions req = let wrapper se @@ -129,6 +135,18 @@ wrapOurExceptions req = InternalException se | otherwise = se in handle $ throwIO . wrapper +#else +wrapOurExceptions :: IO a -> IO a +wrapOurExceptions = + let wrapper se = case fromException se of + Just (_ :: ConnectionRestricted) -> + -- Not really a TLS exception, but there is no + -- way to put SomeException in the + -- InternalIOException this old version uses. + toException $ TlsException se + Nothing -> se + in handle $ throwIO . wrapper +#endif restrictedRawConnection :: Restriction -> IO (Maybe HostAddress -> String -> Int -> IO Connection) restrictedRawConnection cfg = getConnection cfg Nothing From f34faad9aa0ddad78ac9ecf89c7e88b79e131342 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 19 Jun 2018 11:41:50 -0400 Subject: [PATCH 18/41] finalize changelog for release --- CHANGELOG | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 1042448bc9..b80205f9af 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ -git-annex (6.20180622) UNRELEASED; urgency=high +git-annex (6.20180622) upstream; urgency=high + + Security fix release for CVE-2018-10857 - * Security fix release for CVE-2018-10857. * Added annex.security.allowed-url-schemes setting, which defaults to only allowing http, https, and ftp URLs. Note especially that file:/ is no longer enabled by default. This is a security fix. @@ -20,9 +21,7 @@ git-annex (6.20180622) UNRELEASED; urgency=high Only when annex.security.allowed-http-addresses=all will curl and youtube-dl be used. - -- Joey Hess Wed, 30 May 2018 11:49:08 -0400 - -git-annex (6.20180530) UNRELEASED; urgency=medium + Non-security fix changes: * Fix build with ghc 8.4+, which broke due to the Semigroup Monoid change. * version: Show operating system and repository version list @@ -42,7 +41,7 @@ git-annex (6.20180530) UNRELEASED; urgency=medium git-annex fsck --from remote has noticed it's gone, re-running git-annex export or git-annex sync --content will re-upload it. - -- Joey Hess Wed, 30 May 2018 11:49:08 -0400 + -- Joey Hess Tue, 19 Jun 2018 11:40:21 -0400 git-annex (6.20180529) upstream; urgency=medium From 47cd8001bc69cad76d95bbce1e2579bae73168ee Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 19 Jun 2018 14:17:05 -0400 Subject: [PATCH 19/41] call base ManagerSetting's exception wrapper This commit was sponsored by Henrik Riomar on Patreon. --- Utility/HttpManagerRestricted.hs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Utility/HttpManagerRestricted.hs b/Utility/HttpManagerRestricted.hs index 9d39464c7a..bf3434b2d6 100644 --- a/Utility/HttpManagerRestricted.hs +++ b/Utility/HttpManagerRestricted.hs @@ -61,9 +61,9 @@ restrictManagerSettings cfg base = restrictProxy cfg $ base { managerRawConnection = restrictedRawConnection cfg , managerTlsConnection = restrictedTlsConnection cfg #if MIN_VERSION_http_client(0,5,0) - , managerWrapException = wrapOurExceptions + , managerWrapException = wrapOurExceptions base #else - , managerWrapIOException = wrapOurExceptions + , managerWrapIOException = wrapOurExceptions base #endif } @@ -127,17 +127,17 @@ restrictProxy cfg base = do } #if MIN_VERSION_http_client(0,5,0) -wrapOurExceptions :: Request -> IO a -> IO a -wrapOurExceptions req = +wrapOurExceptions :: ManagerSettings -> Request -> IO a -> IO a +wrapOurExceptions base req a = let wrapper se | Just (_ :: ConnectionRestricted) <- fromException se = toException $ HttpExceptionRequest req $ InternalException se | otherwise = se - in handle $ throwIO . wrapper + in managerWrapException base req (handle (throwIO . wrapper) a) #else -wrapOurExceptions :: IO a -> IO a -wrapOurExceptions = +wrapOurExceptions :: ManagerSettings -> IO a -> IO a +wrapOurExceptions base a = let wrapper se = case fromException se of Just (_ :: ConnectionRestricted) -> -- Not really a TLS exception, but there is no @@ -145,7 +145,7 @@ wrapOurExceptions = -- InternalIOException this old version uses. toException $ TlsException se Nothing -> se - in handle $ throwIO . wrapper + in managerWrapIOException base (handle (throwIO . wrapper) a) #endif restrictedRawConnection :: Restriction -> IO (Maybe HostAddress -> String -> Int -> IO Connection) From 923578ad7803d00003f0f8070d8a6cf4cb0b6436 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 19 Jun 2018 14:21:41 -0400 Subject: [PATCH 20/41] improve error message This commit was sponsored by Jack Hill on Patreon. --- Annex/Url.hs | 4 +++- Utility/HttpManagerRestricted.hs | 25 +++++++++++++++---------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/Annex/Url.hs b/Annex/Url.hs index 53a4bfbc34..968118ba99 100644 --- a/Annex/Url.hs +++ b/Annex/Url.hs @@ -75,11 +75,13 @@ getUrlOptions = Annex.getState Annex.urloptions >>= \case | isLoopbackAddress addr = False | isPrivateAddress addr = False | otherwise = True + let connectionrestricted = addrConnectionRestricted + ("Configuration of annex.security.allowed-http-addresses does not allow accessing address " ++) let r = Restriction { addressRestriction = \addr -> if isallowed (addrAddress addr) then Nothing - else Just (addrConnectionRestricted addr) + else Just (connectionrestricted addr) } (settings, pr) <- liftIO $ restrictManagerSettings r U.managerSettings diff --git a/Utility/HttpManagerRestricted.hs b/Utility/HttpManagerRestricted.hs index bf3434b2d6..00611b7b5c 100644 --- a/Utility/HttpManagerRestricted.hs +++ b/Utility/HttpManagerRestricted.hs @@ -16,6 +16,7 @@ module Utility.HttpManagerRestricted ( ConnectionRestricted(..), addrConnectionRestricted, ProxyRestricted(..), + IPAddrString, ) where import Network.HTTP.Client @@ -34,25 +35,29 @@ data Restriction = Restriction { addressRestriction :: AddrInfo -> Maybe ConnectionRestricted } +-- | An exception used to indicate that the connection was restricted. data ConnectionRestricted = ConnectionRestricted String deriving (Show, Typeable) instance Exception ConnectionRestricted -addrConnectionRestricted :: AddrInfo -> ConnectionRestricted -addrConnectionRestricted addr = ConnectionRestricted $ unwords - [ "Configuration does not allow accessing address" - , showSockAddress (addrAddress addr) - ] +type IPAddrString = String + +-- | Constructs a ConnectionRestricted, passing the function a string +-- containing the IP address. +addrConnectionRestricted :: (IPAddrString -> String) -> AddrInfo -> ConnectionRestricted +addrConnectionRestricted mkmessage = + ConnectionRestricted . mkmessage . showSockAddress . addrAddress data ProxyRestricted = ProxyRestricted deriving (Show) --- | Adjusts a ManagerSettings to enforce a Restriction. +-- | Adjusts a ManagerSettings to enforce a Restriction. The restriction +-- will be checked each time a Request is made, and for each redirect +-- followed. -- --- This includes checking the http proxy against the Restriction. --- If access to it is blocked, the ManagerSettings will be made to --- not use the proxy. +-- The http proxy is also checked against the Restriction, and if +-- access to it is blocked, the http proxy will not be used. restrictManagerSettings :: Restriction -> ManagerSettings @@ -219,7 +224,7 @@ convertConnection conn = makeConnection -- For ipv4 and ipv6, the string will contain only the IP address, -- omitting the port that the Show instance includes. -showSockAddress :: SockAddr -> String +showSockAddress :: SockAddr -> IPAddrString showSockAddress a@(SockAddrInet _ _) = takeWhile (/= ':') $ show a showSockAddress a@(SockAddrInet6 _ _ _ _) = From c5166b56afd77eb8bd519ce1990bfb3973fa3f7a Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 19 Jun 2018 17:03:04 -0400 Subject: [PATCH 21/41] second vuln --- ...hole_private_data_exposure_via_addurl.mdwn | 34 +++++++++++-------- doc/security/CVE-2018-10857.mdwn | 32 +++++++++++++---- 2 files changed, 44 insertions(+), 22 deletions(-) diff --git a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn index b27eb64d4e..f1884d8f31 100644 --- a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn +++ b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn @@ -123,30 +123,34 @@ git-annex's url downloading. > to see if they used GETURLS. ipfs did, but not in an exploitable way. > datalad does; looped in its developers to asess. Rest appear not to. -> TODO document this security issue in special remote protocol page - An external special remote might also make a simple http request to a key/value API to download a key, and follow a redirect to file:/// or http://localhost/. If the key uses a cryptographic hash, git-annex verifies the content. But, the attacker could have committed a key that doesn't -use a hash. Also, the attacker could use the hash check as an oracle. +use a hash. Also, the attacker could use the hash check as an oracle, +to guess at the content of files. -What if an external special remote uses https to a hardcoded server (either -fully hardcoded, or configured by the user when they enable the remote), -and uses a http library that only supports https and http (not file:///). -Is that good enough? An attacker would need to either compromise the server -to redirect to a local private webserver, or MITM, and https should prevent -the MITM. +If the external special remote is encrypted, the http content is passed +though gpg. So, whatever location an attacker redirects it to would also +have to be encrypted. gpg is not told what encryption key the content is +expected to be encrypted with. (Could it be told somehow for hybrid and +shared encryption which key to use? pubkey encryption of course uses +the regular gpg public key). -This seems to require auditing of all external special remotes. -git-annex could add a new command to the external protocol, that asks -the special remote if it's been audited, and refuse to use ones that have -not. +So if the attacker knows a file that the user has encrypted with any of +their gpg keys, they can provide that file, and hope it will be decrypted. +Note that this does not need a redirect to a local file or web server; the +attacker can make their web server serve up a gpg encrypted file. -I have emailed all relevant external special remote authors a heads-up and -some questions. +So, content downloaded from encrypted special remotes (both internal and +external) must be rejected unless it can be verified with a hash. Then +content using WORM and URL keys would not be able to be downloaded from +them. Might as well also require a hash check for non-encrypted external +special remotes, to block the redirection attack. There could be a config +setting to say that the git-annex repository is not being shared with +untrusted third parties, and relax that check. > TODO diff --git a/doc/security/CVE-2018-10857.mdwn b/doc/security/CVE-2018-10857.mdwn index bad9edd5fa..88ec3d36d2 100644 --- a/doc/security/CVE-2018-10857.mdwn +++ b/doc/security/CVE-2018-10857.mdwn @@ -1,24 +1,42 @@ CVE-2018-10857: Some uses of git-annex were vulnerable to a private data exposure and exfiltration attack. It could expose the content of files located outside the git-annex repository, or content from a private web -server on localhost or the LAN. +server on localhost or the LAN. Joey Hess discovered this attack. + +Additionally, git-annex encrypted special remotes could be leveraged +by an attacker to decrypt files that were encrypted to the user's gpg +key. This attack could be used to expose encrypted data that was never +stored in git-annex. Daniel Dent discovered this attack in collaboration +with Joey Hess. This was fixed in git-annex 6.20180622. ## details -The attacker needed to have control over one of the remotes of the git-annex +The attacker needs to have control over one of the remotes of the git-annex repository. For example, they may provide a public git-annex repository that the victim clones. Or the victim may have paired repositories with them. Or, equivilantly, the attacker could have read access to the victim's git-annex repository (eg on a server somewhere), and some channel to get commits into it (eg a pull requests). -The attacker does `git-annex addurl --relaxed file:///etc/passwd` and commits -this to the repository in some out of the way place. Then they wait for the -victim to pull the change. (As well as `file:///` urls, the attacker can -use urls to private web servers. The url can also be one that the attacker -controls, that redirects to such urls.) +To perform the private data and exfiltration attack, the attacker +runs `git-annex addurl --relaxed file:///etc/passwd` and commits this to +the repository in some out of the way place. Then they wait for the victim +to pull the change. (As well as `file:///` urls, the attacker can use urls +to private web servers. The url can also be one that the attacker controls, +that redirects to such urls.) + +To perform the gpg decryption attack, the attacker also needs to have +control of an encrypted special remote of the victim's git-annex +repository. The attacker uses `git annex addurl --relaxed` with +an innocuous url, and waits for the user's git-annex to download it, +and upload an (encrypted) copy to the special remote they also control. +At a later point, when the user downloads the content from the special +remote, the attacker instead sends them the content of a gpg encrypted +file they wish to have decrypted in its place. Finally, the attacker +drops their own copy of the original innocuous url, and waits for the user +to send them the decrypted form of the file they earlier sent. The easiest exploit is when the victim is running the git-annex assistant, or is periodically doing `git annex sync --content`. The victim may also perform From 991265e724a6e026ceeb51502e08a4f44f67b486 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 19 Jun 2018 19:41:30 -0400 Subject: [PATCH 22/41] version deps need at least http-client-0.4.31 to build now, and connection-0.2.6 --- .../day_503__security_hole_part_5.mdwn | 17 ++++++++ ...=> CVE-2018-10857_and_CVE-2018-10859.mdwn} | 0 git-annex.cabal | 4 +- .../basement_fix-build-on-android.patch | 41 +++++++++++++++++++ 4 files changed, 60 insertions(+), 2 deletions(-) create mode 100644 doc/devblog/day_503__security_hole_part_5.mdwn rename doc/security/{CVE-2018-10857.mdwn => CVE-2018-10857_and_CVE-2018-10859.mdwn} (100%) create mode 100644 standalone/android/haskell-patches/basement_fix-build-on-android.patch diff --git a/doc/devblog/day_503__security_hole_part_5.mdwn b/doc/devblog/day_503__security_hole_part_5.mdwn new file mode 100644 index 0000000000..88f800e12f --- /dev/null +++ b/doc/devblog/day_503__security_hole_part_5.mdwn @@ -0,0 +1,17 @@ +Started testing that the security fix will build everywhere on +release day. This is being particularly painful for the android build, +which has very old libraries and needed http-client updated, with many +follow-on changes, and is not successfully building yet after 5 hours. +I really need to finish deprecating the android build. + +Pretty exhausted from all this, and thinking what to do about +external special remotes, I elaborated on an idea that Daniel Dent had +raised in discussions about vulnerability, and realized that git-annex +has a second, worse vulnerability. This new one could be used to trick a +git-annex user into decrypting gpg encrypted data that they had +never stored in git-annex. The attacker needs to have control of both an +encrypted special remote and a git remote, so it's not an easy exploit to +pull off, but it's still super bad. + +This week is going to be a lot longer than I thought, and it's already +feeling kind of endless.. diff --git a/doc/security/CVE-2018-10857.mdwn b/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn similarity index 100% rename from doc/security/CVE-2018-10857.mdwn rename to doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn diff --git a/git-annex.cabal b/git-annex.cabal index bab975c26a..fc64a16651 100644 --- a/git-annex.cabal +++ b/git-annex.cabal @@ -340,8 +340,8 @@ Executable git-annex bloomfilter, edit-distance, resourcet, - connection, - http-client, + connection (>= 0.2.6), + http-client (>= 0.4.31), http-client-tls, http-types (>= 0.7), http-conduit (>= 2.0), diff --git a/standalone/android/haskell-patches/basement_fix-build-on-android.patch b/standalone/android/haskell-patches/basement_fix-build-on-android.patch new file mode 100644 index 0000000000..948473bc2e --- /dev/null +++ b/standalone/android/haskell-patches/basement_fix-build-on-android.patch @@ -0,0 +1,41 @@ +From cc0c373b69f93057cbdcb634a461e10ec019d87a Mon Sep 17 00:00:00 2001 +From: androidbuilder +Date: Wed, 20 Jun 2018 00:29:11 +0100 +Subject: [PATCH] fix build on android + +--- + Basement/Terminal.hs | 2 -- + basement.cabal | 1 - + 2 files changed, 3 deletions(-) + +diff --git a/Basement/Terminal.hs b/Basement/Terminal.hs +index 8136e52..cca9606 100644 +--- a/Basement/Terminal.hs ++++ b/Basement/Terminal.hs +@@ -1,11 +1,9 @@ + {-# LANGUAGE CPP #-} + module Basement.Terminal + ( initialize +- , getDimensions + ) where + + import Basement.Compat.Base +-import Basement.Terminal.Size (getDimensions) + #ifdef mingw32_HOST_OS + import System.IO (hSetEncoding, utf8, hPutStrLn, stderr, stdin, stdout) + import System.Win32.Console (setConsoleCP, setConsoleOutputCP, getConsoleCP, getConsoleOutputCP) +diff --git a/basement.cabal b/basement.cabal +index af50291..0824c94 100644 +--- a/basement.cabal ++++ b/basement.cabal +@@ -135,7 +135,6 @@ library + Basement.String.Encoding.ASCII7 + Basement.String.Encoding.ISO_8859_1 + +- Basement.Terminal.Size + + + build-depends: base >= 4.7 && < 5 +-- +2.1.4 + From 22f49f216e02a1a8a8a6f23445d4e81bfc0db5b1 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Wed, 20 Jun 2018 13:05:02 -0400 Subject: [PATCH 23/41] get android building the security fix Had to update http-client and network, with follow-on dep changes. This commit was sponsored by Brock Spratlen on Patreon. --- Utility/Url.hs | 2 +- standalone/android/cabal.config | 57 +-- ....BSD-symbols-not-available-in-bionic.patch | 157 -------- ....0_0003-configure-misdetects-accept4.patch | 34 -- ...-getprotobyname-hack-for-tcp-and-udp.patch | 28 -- ...k_2.4.1.0_0005-no-NODELAY-on-android.patch | 25 -- ...work_2.5.0.0_0001-android-port-fixes.patch | 161 --------- .../network_android-port-fixes.patch | 341 ++++++++++++++++++ .../warp_remove-ipv6-stuff.patch | 39 ++ ...509-store_support-Android-cert-store.patch | 28 ++ ...09-system_support-Android-cert-store.patch | 27 -- .../haskell-patches/xss-sanitize_deps.patch | 24 ++ standalone/android/install-haskell-packages | 6 +- 13 files changed, 469 insertions(+), 460 deletions(-) delete mode 100644 standalone/android/haskell-patches/network_2.4.1.0_0002-remove-Network.BSD-symbols-not-available-in-bionic.patch delete mode 100644 standalone/android/haskell-patches/network_2.4.1.0_0003-configure-misdetects-accept4.patch delete mode 100644 standalone/android/haskell-patches/network_2.4.1.0_0004-getprotobyname-hack-for-tcp-and-udp.patch delete mode 100644 standalone/android/haskell-patches/network_2.4.1.0_0005-no-NODELAY-on-android.patch delete mode 100644 standalone/android/haskell-patches/network_2.5.0.0_0001-android-port-fixes.patch create mode 100644 standalone/android/haskell-patches/network_android-port-fixes.patch create mode 100644 standalone/android/haskell-patches/warp_remove-ipv6-stuff.patch create mode 100644 standalone/android/haskell-patches/x509-store_support-Android-cert-store.patch delete mode 100644 standalone/android/haskell-patches/x509-system_support-Android-cert-store.patch create mode 100644 standalone/android/haskell-patches/xss-sanitize_deps.patch diff --git a/Utility/Url.hs b/Utility/Url.hs index 3301a4b0ef..31dca28154 100644 --- a/Utility/Url.hs +++ b/Utility/Url.hs @@ -49,7 +49,7 @@ import qualified Data.ByteString.Lazy as L import qualified Data.Set as S import Control.Monad.Trans.Resource import Network.HTTP.Conduit -import Network.HTTP.Client (brRead, withResponse) +import Network.HTTP.Client import Data.Conduit #if ! MIN_VERSION_http_client(0,5,0) diff --git a/standalone/android/cabal.config b/standalone/android/cabal.config index f61fe9a0b9..1313b1c2e4 100644 --- a/standalone/android/cabal.config +++ b/standalone/android/cabal.config @@ -1,4 +1,11 @@ constraints: unix installed, + blaze-html ==0.8.1.3, + blaze-markup ==0.7.0.3, + basement ==0.0.7, + memory ==0.14.9, + connection ==0.2.8, + aws ==0.13.0, + lifted-base ==0.2.3.6, Crypto ==4.2.5.1, binary ==0.7.6.1, DAV ==1.0.3, @@ -6,16 +13,16 @@ constraints: unix installed, HUnit ==1.2.5.2, IfElse ==0.85, MissingH ==1.2.1.0, - directory ==1.2.2.0, + directory ==1.2.2.0, MonadRandom ==0.1.13, QuickCheck ==2.7.6, SafeSemaphore ==0.10.1, aeson ==0.7.0.6, ansi-wl-pprint ==0.6.7.1, appar ==0.1.4, - asn1-encoding ==0.8.1.3, - asn1-parse ==0.8.1, - asn1-types ==0.2.3, + asn1-encoding ==0.9.5, + asn1-parse ==0.9.4, + asn1-types ==0.3.2, async ==2.0.1.5, attoparsec ==0.11.3.4, attoparsec-conduit ==1.1.0, @@ -36,7 +43,6 @@ constraints: unix installed, comonad ==4.2, conduit ==1.1.6, conduit-extra ==1.1.3, - connection ==0.2.3, contravariant ==0.6.1.1, cookie ==0.4.1.2, cprng-aes ==0.5.2, @@ -44,7 +50,7 @@ constraints: unix installed, crypto-cipher-types ==0.0.9, crypto-numbers ==0.2.3, crypto-pubkey ==0.2.4, - crypto-pubkey-types ==0.4.2.2, + crypto-pubkey-types ==0.4.2.3, crypto-random ==0.0.7, cryptohash ==0.11.6, cryptohash-conduit ==0.1.1, @@ -80,19 +86,19 @@ constraints: unix installed, hashable ==1.2.1.0, hinotify ==0.3.5, hjsmin ==0.1.4.7, - hslogger ==1.2.1, - http-client ==0.4.11.1, - http-client-tls ==0.2.2, - http-conduit ==2.1.5, - http-date ==0.0.2, - http-types ==0.8.5, - blaze-builder ==0.3.3.2, + hslogger ==1.2.10, + http-client ==0.4.31.1, + http-client-tls ==0.2.4.1, + http-conduit ==2.1.9, + http-date ==0.0.6.1, + http-types ==0.9.1, + blaze-builder ==0.3.3.2, hxt ==9.3.1.4, hxt-charproperties ==9.1.1.1, hxt-regex-xmlschema ==9.0.4, hxt-unicode ==9.0.2.2, idna ==0.2, - iproute ==1.2.11, + iproute ==1.3.1, json ==0.5, keys ==3.10.1, language-javascript ==0.5.13, @@ -107,7 +113,7 @@ constraints: unix installed, monads-tf ==0.1.0.2, mtl ==2.1.2, nats ==0.1.2, - network ==2.4.1.2, + network ==2.6.3.1 network-conduit ==1.1.0, network-info ==0.2.0.5, network-multicast ==0.0.10, @@ -141,7 +147,7 @@ constraints: unix installed, silently ==1.2.4.1, simple-sendfile ==0.2.14, skein ==1.0.9, - socks ==0.5.4, + socks ==0.5.5, split ==0.2.2, stm ==2.4.2, stm-chans ==3.0.0.2, @@ -157,7 +163,7 @@ constraints: unix installed, text ==1.1.1.0, text-icu ==0.6.3.7, tf-random ==0.5, - tls ==1.2.9, + tls ==1.3.8, transformers ==0.3.0.0, transformers-base ==0.4.1, transformers-compat ==0.3.3.3, @@ -174,13 +180,13 @@ constraints: unix installed, wai-app-static ==3.0.0.1, wai-extra ==3.0.1.2, wai-logger ==2.1.1, - warp ==3.0.0.5, - warp-tls ==3.0.0, + warp ==3.0.8, + warp-tls ==3.0.4.2, word8 ==0.1.1, - x509 ==1.4.11, - x509-store ==1.4.4, - x509-system ==1.4.5, - x509-validation ==1.5.0, + x509 ==1.6.5, + x509-store ==1.6.3, + x509-system ==1.6.6, + x509-validation ==1.6.8, xml ==1.3.13, xml-conduit ==1.2.1, xml-hamlet ==0.4.0.9, @@ -197,6 +203,7 @@ constraints: unix installed, yesod-static ==1.2.4, zlib ==0.5.4.1, bytestring installed, + process ==1.2.3.0, scientific ==0.3.3.1, - clock ==0.4.6.0 - cryptonite ==cryptonite-0.16 + clock ==0.4.6.0, + cryptonite ==0.15 diff --git a/standalone/android/haskell-patches/network_2.4.1.0_0002-remove-Network.BSD-symbols-not-available-in-bionic.patch b/standalone/android/haskell-patches/network_2.4.1.0_0002-remove-Network.BSD-symbols-not-available-in-bionic.patch deleted file mode 100644 index 5b07f233b6..0000000000 --- a/standalone/android/haskell-patches/network_2.4.1.0_0002-remove-Network.BSD-symbols-not-available-in-bionic.patch +++ /dev/null @@ -1,157 +0,0 @@ -From 7861b133bb269b50fcf709291449cb0473818902 Mon Sep 17 00:00:00 2001 -From: Joey Hess -Date: Sun, 29 Dec 2013 21:29:23 +0000 -Subject: [PATCH] remove Network.BSD symbols not available in bionic - ---- - Network/BSD.hsc | 98 ------------------------------------------------------- - 1 file changed, 98 deletions(-) - -diff --git a/Network/BSD.hsc b/Network/BSD.hsc -index d6dae85..27910f4 100644 ---- a/Network/BSD.hsc -+++ b/Network/BSD.hsc -@@ -30,15 +30,6 @@ module Network.BSD - , getHostByAddr - , hostAddress - --#if defined(HAVE_GETHOSTENT) && !defined(cygwin32_HOST_OS) && !defined(mingw32_HOST_OS) && !defined(_WIN32) -- , getHostEntries -- -- -- ** Low level functionality -- , setHostEntry -- , getHostEntry -- , endHostEntry --#endif -- - -- * Service names - , ServiceEntry(..) - , ServiceName -@@ -64,14 +55,6 @@ module Network.BSD - , getProtocolNumber - , defaultProtocol - --#if !defined(cygwin32_HOST_OS) && !defined(mingw32_HOST_OS) && !defined(_WIN32) -- , getProtocolEntries -- -- ** Low level functionality -- , setProtocolEntry -- , getProtocolEntry -- , endProtocolEntry --#endif -- - -- * Port numbers - , PortNumber - -@@ -83,11 +66,7 @@ module Network.BSD - #if !defined(cygwin32_HOST_OS) && !defined(mingw32_HOST_OS) && !defined(_WIN32) - , getNetworkByName - , getNetworkByAddr -- , getNetworkEntries - -- ** Low level functionality -- , setNetworkEntry -- , getNetworkEntry -- , endNetworkEntry - #endif - ) where - -@@ -303,31 +282,6 @@ getProtocolNumber proto = do - (ProtocolEntry _ _ num) <- getProtocolByName proto - return num - --#if !defined(cygwin32_HOST_OS) && !defined(mingw32_HOST_OS) && !defined(_WIN32) --getProtocolEntry :: IO ProtocolEntry -- Next Protocol Entry from DB --getProtocolEntry = withLock $ do -- ent <- throwNoSuchThingIfNull "getProtocolEntry" "no such protocol entry" -- $ trySysCall c_getprotoent -- peek ent -- --foreign import ccall unsafe "getprotoent" c_getprotoent :: IO (Ptr ProtocolEntry) -- --setProtocolEntry :: Bool -> IO () -- Keep DB Open ? --setProtocolEntry flg = withLock $ trySysCall $ c_setprotoent (fromBool flg) -- --foreign import ccall unsafe "setprotoent" c_setprotoent :: CInt -> IO () -- --endProtocolEntry :: IO () --endProtocolEntry = withLock $ trySysCall $ c_endprotoent -- --foreign import ccall unsafe "endprotoent" c_endprotoent :: IO () -- --getProtocolEntries :: Bool -> IO [ProtocolEntry] --getProtocolEntries stayOpen = withLock $ do -- setProtocolEntry stayOpen -- getEntries (getProtocolEntry) (endProtocolEntry) --#endif -- - -- --------------------------------------------------------------------------- - -- Host lookups - -@@ -402,31 +356,6 @@ getHostByAddr family addr = do - foreign import CALLCONV safe "gethostbyaddr" - c_gethostbyaddr :: Ptr HostAddress -> CInt -> CInt -> IO (Ptr HostEntry) - --#if defined(HAVE_GETHOSTENT) && !defined(cygwin32_HOST_OS) && !defined(mingw32_HOST_OS) && !defined(_WIN32) --getHostEntry :: IO HostEntry --getHostEntry = withLock $ do -- throwNoSuchThingIfNull "getHostEntry" "unable to retrieve host entry" -- $ trySysCall $ c_gethostent -- >>= peek -- --foreign import ccall unsafe "gethostent" c_gethostent :: IO (Ptr HostEntry) -- --setHostEntry :: Bool -> IO () --setHostEntry flg = withLock $ trySysCall $ c_sethostent (fromBool flg) -- --foreign import ccall unsafe "sethostent" c_sethostent :: CInt -> IO () -- --endHostEntry :: IO () --endHostEntry = withLock $ c_endhostent -- --foreign import ccall unsafe "endhostent" c_endhostent :: IO () -- --getHostEntries :: Bool -> IO [HostEntry] --getHostEntries stayOpen = do -- setHostEntry stayOpen -- getEntries (getHostEntry) (endHostEntry) --#endif -- - -- --------------------------------------------------------------------------- - -- Accessing network information - -@@ -488,33 +417,6 @@ getNetworkByAddr addr family = withLock $ do - foreign import ccall unsafe "getnetbyaddr" - c_getnetbyaddr :: NetworkAddr -> CInt -> IO (Ptr NetworkEntry) - --getNetworkEntry :: IO NetworkEntry --getNetworkEntry = withLock $ do -- throwNoSuchThingIfNull "getNetworkEntry" "no more network entries" -- $ trySysCall $ c_getnetent -- >>= peek -- --foreign import ccall unsafe "getnetent" c_getnetent :: IO (Ptr NetworkEntry) -- ---- | Open the network name database. The parameter specifies ---- whether a connection is maintained open between various ---- networkEntry calls --setNetworkEntry :: Bool -> IO () --setNetworkEntry flg = withLock $ trySysCall $ c_setnetent (fromBool flg) -- --foreign import ccall unsafe "setnetent" c_setnetent :: CInt -> IO () -- ---- | Close the connection to the network name database. --endNetworkEntry :: IO () --endNetworkEntry = withLock $ trySysCall $ c_endnetent -- --foreign import ccall unsafe "endnetent" c_endnetent :: IO () -- ---- | Get the list of network entries. --getNetworkEntries :: Bool -> IO [NetworkEntry] --getNetworkEntries stayOpen = do -- setNetworkEntry stayOpen -- getEntries (getNetworkEntry) (endNetworkEntry) - #endif - - -- Mutex for name service lockdown --- -1.7.10.4 - diff --git a/standalone/android/haskell-patches/network_2.4.1.0_0003-configure-misdetects-accept4.patch b/standalone/android/haskell-patches/network_2.4.1.0_0003-configure-misdetects-accept4.patch deleted file mode 100644 index 084d355ba3..0000000000 --- a/standalone/android/haskell-patches/network_2.4.1.0_0003-configure-misdetects-accept4.patch +++ /dev/null @@ -1,34 +0,0 @@ -From 478fc7ae42030c1345e75727e54e1f8f895d3e22 Mon Sep 17 00:00:00 2001 -From: dummy -Date: Wed, 15 Oct 2014 15:16:21 +0000 -Subject: [PATCH] avoid accept4 - ---- - Network/Socket.hsc | 4 ++-- - 1 file changed, 2 insertions(+), 2 deletions(-) - -diff --git a/Network/Socket.hsc b/Network/Socket.hsc -index 2fe62ee..94db7a4 100644 ---- a/Network/Socket.hsc -+++ b/Network/Socket.hsc -@@ -511,7 +511,7 @@ accept sock@(MkSocket s family stype protocol status) = do - #else - with (fromIntegral sz) $ \ ptr_len -> do - new_sock <- --# ifdef HAVE_ACCEPT4 -+#if 0 - throwSocketErrorIfMinus1RetryMayBlock "accept" - (threadWaitRead (fromIntegral s)) - (c_accept4 s sockaddr ptr_len (#const SOCK_NONBLOCK)) -@@ -1602,7 +1602,7 @@ foreign import CALLCONV SAFE_ON_WIN "connect" - c_connect :: CInt -> Ptr SockAddr -> CInt{-CSockLen???-} -> IO CInt - foreign import CALLCONV unsafe "accept" - c_accept :: CInt -> Ptr SockAddr -> Ptr CInt{-CSockLen???-} -> IO CInt --#ifdef HAVE_ACCEPT4 -+#if 0 - foreign import CALLCONV unsafe "accept4" - c_accept4 :: CInt -> Ptr SockAddr -> Ptr CInt{-CSockLen???-} -> CInt -> IO CInt - #endif --- -2.1.1 - diff --git a/standalone/android/haskell-patches/network_2.4.1.0_0004-getprotobyname-hack-for-tcp-and-udp.patch b/standalone/android/haskell-patches/network_2.4.1.0_0004-getprotobyname-hack-for-tcp-and-udp.patch deleted file mode 100644 index 4cc22cbcac..0000000000 --- a/standalone/android/haskell-patches/network_2.4.1.0_0004-getprotobyname-hack-for-tcp-and-udp.patch +++ /dev/null @@ -1,28 +0,0 @@ -From b1a581007759e2d9e53ef776e4f10d1de87b8377 Mon Sep 17 00:00:00 2001 -From: Joey Hess -Date: Tue, 7 May 2013 14:51:09 -0400 -Subject: [PATCH] getprotobyname hack for tcp and udp - -Otherwise, core network stuff fails to get the numbers for these protocols. ---- - Network/BSD.hsc | 4 ++++ - 1 file changed, 4 insertions(+) - -diff --git a/Network/BSD.hsc b/Network/BSD.hsc -index f0c9f5b..a289143 100644 ---- a/Network/BSD.hsc -+++ b/Network/BSD.hsc -@@ -259,6 +259,10 @@ instance Storable ProtocolEntry where - poke _p = error "Storable.poke(BSD.ProtocolEntry) not implemented" - - getProtocolByName :: ProtocolName -> IO ProtocolEntry -+getProtocolByName "tcp" = return $ -+ ProtocolEntry {protoName = "tcp", protoAliases = ["TCP"], protoNumber = 6} -+getProtocolByName "udp" = return $ -+ ProtocolEntry {protoName = "udp", protoAliases = ["UDP"], protoNumber = 17} - getProtocolByName name = withLock $ do - withCString name $ \ name_cstr -> do - throwNoSuchThingIfNull "getProtocolByName" ("no such protocol name: " ++ name) --- -1.8.2.rc3 - diff --git a/standalone/android/haskell-patches/network_2.4.1.0_0005-no-NODELAY-on-android.patch b/standalone/android/haskell-patches/network_2.4.1.0_0005-no-NODELAY-on-android.patch deleted file mode 100644 index da4a71af21..0000000000 --- a/standalone/android/haskell-patches/network_2.4.1.0_0005-no-NODELAY-on-android.patch +++ /dev/null @@ -1,25 +0,0 @@ -From bfecbc7bd09cbbebdef12aa525dc17109326db3f Mon Sep 17 00:00:00 2001 -From: Joey Hess -Date: Sun, 29 Dec 2013 21:31:07 +0000 -Subject: [PATCH] no NODELAY on android - ---- - Network/Socket.hsc | 2 +- - 1 file changed, 1 insertion(+), 1 deletion(-) - -diff --git a/Network/Socket.hsc b/Network/Socket.hsc -index 6c21209..c360889 100644 ---- a/Network/Socket.hsc -+++ b/Network/Socket.hsc -@@ -923,7 +923,7 @@ packSocketOption so = - Just MaxSegment -> Just ((#const IPPROTO_TCP), (#const TCP_MAXSEG)) - #endif - #ifdef TCP_NODELAY -- Just NoDelay -> Just ((#const IPPROTO_TCP), (#const TCP_NODELAY)) -+ Just NoDelay -> Nothing -- Just ((#const IPPROTO_TCP), (#const TCP_NODELAY)) - #endif - #ifdef TCP_CORK - Just Cork -> Just ((#const IPPROTO_TCP), (#const TCP_CORK)) --- -1.7.10.4 - diff --git a/standalone/android/haskell-patches/network_2.5.0.0_0001-android-port-fixes.patch b/standalone/android/haskell-patches/network_2.5.0.0_0001-android-port-fixes.patch deleted file mode 100644 index 325b89fb4c..0000000000 --- a/standalone/android/haskell-patches/network_2.5.0.0_0001-android-port-fixes.patch +++ /dev/null @@ -1,161 +0,0 @@ -From b3cb294077b627892721a2ebf9e0ce81f35f8c4c Mon Sep 17 00:00:00 2001 -From: dummy -Date: Sun, 25 May 2014 09:28:45 +0200 -Subject: [PATCH] android port fixes - -Build note: Ensure a hsc2hs in PATH is modified to pass -x to the real -one, to enable cross-compiling. ---- - Network/Socket.hsc | 22 ++++++---------------- - Network/Socket/ByteString.hsc | 2 +- - Network/Socket/Internal.hsc | 2 +- - Network/Socket/Types.hsc | 4 ++-- - cbits/HsNet.c | 14 ++++++++++++++ - configure | 1 + - 6 files changed, 25 insertions(+), 20 deletions(-) - -diff --git a/Network/Socket.hsc b/Network/Socket.hsc -index 607b270..04a83e8 100644 ---- a/Network/Socket.hsc -+++ b/Network/Socket.hsc -@@ -35,7 +35,7 @@ module Network.Socket - , SockAddr(..) - , SocketStatus(..) - , HostAddress --#if defined(IPV6_SOCKET_SUPPORT) -+#if defined(IPV6_SOCKET_SUPPORTNO) - , HostAddress6 - , FlowInfo - , ScopeID -@@ -52,7 +52,7 @@ module Network.Socket - , HostName - , ServiceName - --#if defined(IPV6_SOCKET_SUPPORT) -+#if defined(IPV6_SOCKET_SUPPORT) || 1 - , AddrInfo(..) - - , AddrInfoFlag(..) -@@ -134,7 +134,7 @@ module Network.Socket - -- * Special constants - , aNY_PORT - , iNADDR_ANY --#if defined(IPV6_SOCKET_SUPPORT) -+#if defined(IPV6_SOCKET_SUPPORTNO) - , iN6ADDR_ANY - #endif - , sOMAXCONN -@@ -326,16 +326,6 @@ socket family stype protocol = do - setNonBlockIfNeeded fd - socket_status <- newMVar NotConnected - let sock = MkSocket fd family stype protocol socket_status --#if HAVE_DECL_IPV6_V6ONLY --# if defined(mingw32_HOST_OS) -- -- the IPv6Only option is only supported on Windows Vista and later, -- -- so trying to change it might throw an error -- when (family == AF_INET6) $ -- E.catch (setSocketOption sock IPv6Only 0) $ (\(_ :: E.IOException) -> return ()) --# else -- when (family == AF_INET6) $ setSocketOption sock IPv6Only 0 --# endif --#endif - return sock - - -- | Build a pair of connected socket objects using the given address -@@ -1061,9 +1051,9 @@ aNY_PORT = 0 - iNADDR_ANY :: HostAddress - iNADDR_ANY = htonl (#const INADDR_ANY) - --foreign import CALLCONV unsafe "htonl" htonl :: Word32 -> Word32 -+foreign import CALLCONV unsafe "my_htonl" htonl :: Word32 -> Word32 - --#if defined(IPV6_SOCKET_SUPPORT) -+#if defined(IPV6_SOCKET_SUPPORTNO) - -- | The IPv6 wild card address. - - iN6ADDR_ANY :: HostAddress6 -@@ -1241,7 +1231,7 @@ unpackBits ((k,v):xs) r - ----------------------------------------------------------------------------- - -- Address and service lookups - --#if defined(IPV6_SOCKET_SUPPORT) -+#if defined(IPV6_SOCKET_SUPPORT) || 1 - - -- | Flags that control the querying behaviour of 'getAddrInfo'. - data AddrInfoFlag -diff --git a/Network/Socket/ByteString.hsc b/Network/Socket/ByteString.hsc -index e21ad1b..c2dd70a 100644 ---- a/Network/Socket/ByteString.hsc -+++ b/Network/Socket/ByteString.hsc -@@ -197,7 +197,7 @@ sendMany sock@(MkSocket fd _ _ _ _) cs = do - liftM fromIntegral . withIOVec cs $ \(iovsPtr, iovsLen) -> - throwSocketErrorWaitWrite sock "writev" $ - c_writev (fromIntegral fd) iovsPtr -- (fromIntegral (min iovsLen (#const IOV_MAX))) -+ (fromIntegral (min iovsLen (0x0026))) - #else - sendMany sock = sendAll sock . B.concat - #endif -diff --git a/Network/Socket/Internal.hsc b/Network/Socket/Internal.hsc -index 83333f7..0dd6a7d 100644 ---- a/Network/Socket/Internal.hsc -+++ b/Network/Socket/Internal.hsc -@@ -24,7 +24,7 @@ module Network.Socket.Internal - ( - -- * Socket addresses - HostAddress --#if defined(IPV6_SOCKET_SUPPORT) -+#if defined(IPV6_SOCKET_SUPPORTNO) - , HostAddress6 - , FlowInfo - , ScopeID -diff --git a/Network/Socket/Types.hsc b/Network/Socket/Types.hsc -index 48a43bb..1c5994f 100644 ---- a/Network/Socket/Types.hsc -+++ b/Network/Socket/Types.hsc -@@ -711,8 +711,8 @@ intToPortNumber v = PortNum (htons (fromIntegral v)) - portNumberToInt :: PortNumber -> Int - portNumberToInt (PortNum po) = fromIntegral (ntohs po) - --foreign import CALLCONV unsafe "ntohs" ntohs :: Word16 -> Word16 --foreign import CALLCONV unsafe "htons" htons :: Word16 -> Word16 -+foreign import CALLCONV unsafe "my_ntohs" ntohs :: Word16 -> Word16 -+foreign import CALLCONV unsafe "my_htons" htons :: Word16 -> Word16 - --foreign import CALLCONV unsafe "ntohl" ntohl :: Word32 -> Word32 - - instance Enum PortNumber where -diff --git a/cbits/HsNet.c b/cbits/HsNet.c -index 86b55dc..5ea1199 100644 ---- a/cbits/HsNet.c -+++ b/cbits/HsNet.c -@@ -6,3 +6,17 @@ - - #define INLINE - #include "HsNet.h" -+ -+#include -+uint16_t my_htons(uint16_t v) -+{ -+ htons(v); -+} -+uint32_t my_htonl(uint32_t v) -+{ -+ htonl(v); -+} -+uint16_t my_ntohs(uint16_t v) -+{ -+ ntohs(v); -+} -diff --git a/configure b/configure -index db8240d..41674d9 100755 ---- a/configure -+++ b/configure -@@ -1,4 +1,5 @@ - #! /bin/sh -+set -- --host=arm-linux-androideabi - # Guess values for system-dependent variables and create Makefiles. - # Generated by GNU Autoconf 2.69 for Haskell network package 2.3.0.14. - # --- -2.0.0.rc2 - diff --git a/standalone/android/haskell-patches/network_android-port-fixes.patch b/standalone/android/haskell-patches/network_android-port-fixes.patch new file mode 100644 index 0000000000..784eec13a9 --- /dev/null +++ b/standalone/android/haskell-patches/network_android-port-fixes.patch @@ -0,0 +1,341 @@ +From 834a0d3bfe56b969a65eff834604442cde8798f7 Mon Sep 17 00:00:00 2001 +From: dummy +Date: Wed, 20 Jun 2018 05:06:41 +0100 +Subject: [PATCH] android port fixes + +Build note: Ensure a hsc2hs in PATH is modified to pass -x to the real +one, to enable cross-compiling. +--- + Network/BSD.hsc | 84 ------------------------------------------- + Network/Socket.hsc | 16 ++++----- + Network/Socket/ByteString.hsc | 2 +- + Network/Socket/Internal.hsc | 2 +- + Network/Socket/Types.hsc | 14 +++----- + cbits/HsNet.c | 18 ++++++++++ + configure | 1 + + include/HsNetworkConfig.h | 4 +-- + 8 files changed, 36 insertions(+), 105 deletions(-) + +diff --git a/Network/BSD.hsc b/Network/BSD.hsc +index 67f2fcd..4c86af5 100644 +--- a/Network/BSD.hsc ++++ b/Network/BSD.hsc +@@ -28,12 +28,8 @@ module Network.BSD + , hostAddress + + #if defined(HAVE_GETHOSTENT) && !defined(mingw32_HOST_OS) +- , getHostEntries +- + -- ** Low level functionality +- , setHostEntry + , getHostEntry +- , endHostEntry + #endif + + -- * Service names +@@ -61,14 +57,6 @@ module Network.BSD + , getProtocolNumber + , defaultProtocol + +-#if !defined(mingw32_HOST_OS) +- , getProtocolEntries +- -- ** Low level functionality +- , setProtocolEntry +- , getProtocolEntry +- , endProtocolEntry +-#endif +- + -- * Port numbers + , PortNumber + +@@ -80,11 +68,6 @@ module Network.BSD + #if !defined(mingw32_HOST_OS) + , getNetworkByName + , getNetworkByAddr +- , getNetworkEntries +- -- ** Low level functionality +- , setNetworkEntry +- , getNetworkEntry +- , endNetworkEntry + #endif + + #if defined(HAVE_IF_NAMETOINDEX) +@@ -298,30 +281,6 @@ getProtocolNumber proto = do + (ProtocolEntry _ _ num) <- getProtocolByName proto + return num + +-#if !defined(mingw32_HOST_OS) +-getProtocolEntry :: IO ProtocolEntry -- Next Protocol Entry from DB +-getProtocolEntry = withLock $ do +- ent <- throwNoSuchThingIfNull "getProtocolEntry" "no such protocol entry" +- $ c_getprotoent +- peek ent +- +-foreign import ccall unsafe "getprotoent" c_getprotoent :: IO (Ptr ProtocolEntry) +- +-setProtocolEntry :: Bool -> IO () -- Keep DB Open ? +-setProtocolEntry flg = withLock $ c_setprotoent (fromBool flg) +- +-foreign import ccall unsafe "setprotoent" c_setprotoent :: CInt -> IO () +- +-endProtocolEntry :: IO () +-endProtocolEntry = withLock $ c_endprotoent +- +-foreign import ccall unsafe "endprotoent" c_endprotoent :: IO () +- +-getProtocolEntries :: Bool -> IO [ProtocolEntry] +-getProtocolEntries stayOpen = withLock $ do +- setProtocolEntry stayOpen +- getEntries (getProtocolEntry) (endProtocolEntry) +-#endif + + -- --------------------------------------------------------------------------- + -- Host lookups +@@ -405,21 +364,6 @@ getHostEntry = withLock $ do + >>= peek + + foreign import ccall unsafe "gethostent" c_gethostent :: IO (Ptr HostEntry) +- +-setHostEntry :: Bool -> IO () +-setHostEntry flg = withLock $ c_sethostent (fromBool flg) +- +-foreign import ccall unsafe "sethostent" c_sethostent :: CInt -> IO () +- +-endHostEntry :: IO () +-endHostEntry = withLock $ c_endhostent +- +-foreign import ccall unsafe "endhostent" c_endhostent :: IO () +- +-getHostEntries :: Bool -> IO [HostEntry] +-getHostEntries stayOpen = do +- setHostEntry stayOpen +- getEntries (getHostEntry) (endHostEntry) + #endif + + -- --------------------------------------------------------------------------- +@@ -482,34 +426,6 @@ getNetworkByAddr addr family = withLock $ do + + foreign import ccall unsafe "getnetbyaddr" + c_getnetbyaddr :: NetworkAddr -> CInt -> IO (Ptr NetworkEntry) +- +-getNetworkEntry :: IO NetworkEntry +-getNetworkEntry = withLock $ do +- throwNoSuchThingIfNull "getNetworkEntry" "no more network entries" +- $ c_getnetent +- >>= peek +- +-foreign import ccall unsafe "getnetent" c_getnetent :: IO (Ptr NetworkEntry) +- +--- | Open the network name database. The parameter specifies +--- whether a connection is maintained open between various +--- networkEntry calls +-setNetworkEntry :: Bool -> IO () +-setNetworkEntry flg = withLock $ c_setnetent (fromBool flg) +- +-foreign import ccall unsafe "setnetent" c_setnetent :: CInt -> IO () +- +--- | Close the connection to the network name database. +-endNetworkEntry :: IO () +-endNetworkEntry = withLock $ c_endnetent +- +-foreign import ccall unsafe "endnetent" c_endnetent :: IO () +- +--- | Get the list of network entries. +-getNetworkEntries :: Bool -> IO [NetworkEntry] +-getNetworkEntries stayOpen = do +- setNetworkEntry stayOpen +- getEntries (getNetworkEntry) (endNetworkEntry) + #endif + + -- --------------------------------------------------------------------------- +diff --git a/Network/Socket.hsc b/Network/Socket.hsc +index 8b2e6fe..b02b80d 100644 +--- a/Network/Socket.hsc ++++ b/Network/Socket.hsc +@@ -59,7 +59,7 @@ module Network.Socket + , HostName + , ServiceName + +-#if defined(IPV6_SOCKET_SUPPORT) ++#if defined(IPV6_SOCKET_SUPPORT) || 1 + , AddrInfo(..) + + , AddrInfoFlag(..) +@@ -143,7 +143,7 @@ module Network.Socket + -- * Special constants + , aNY_PORT + , iNADDR_ANY +-#if defined(IPV6_SOCKET_SUPPORT) ++#if defined(IPV6_SOCKET_SUPPORTNO) + , iN6ADDR_ANY + #endif + , sOMAXCONN +@@ -521,7 +521,7 @@ accept sock@(MkSocket s family stype protocol status) = do + return new_sock + #else + with (fromIntegral sz) $ \ ptr_len -> do +-# ifdef HAVE_ACCEPT4 ++#if 0 + new_sock <- throwSocketErrorIfMinus1RetryMayBlock "accept" + (threadWaitRead (fromIntegral s)) + (c_accept4 s sockaddr ptr_len (#const SOCK_NONBLOCK)) +@@ -903,7 +903,7 @@ packSocketOption so = + Just MaxSegment -> Just ((#const IPPROTO_TCP), (#const TCP_MAXSEG)) + #endif + #ifdef TCP_NODELAY +- Just NoDelay -> Just ((#const IPPROTO_TCP), (#const TCP_NODELAY)) ++ Just NoDelay -> Nothing -- Just ((#const IPPROTO_TCP), (#const TCP_NODELAY)) + #endif + #ifdef TCP_USER_TIMEOUT + Just UserTimeout -> Just ((#const IPPROTO_TCP), (#const TCP_USER_TIMEOUT)) +@@ -1036,9 +1036,9 @@ iNADDR_ANY :: HostAddress + iNADDR_ANY = htonl (#const INADDR_ANY) + + -- | Converts the from host byte order to network byte order. +-foreign import CALLCONV unsafe "htonl" htonl :: Word32 -> Word32 ++foreign import CALLCONV unsafe "my_htonl" htonl :: Word32 -> Word32 + -- | Converts the from network byte order to host byte order. +-foreign import CALLCONV unsafe "ntohl" ntohl :: Word32 -> Word32 ++foreign import CALLCONV unsafe "my_ntohl" ntohl :: Word32 -> Word32 + + #if defined(IPV6_SOCKET_SUPPORT) + -- | The IPv6 wild card address. +@@ -1206,7 +1206,7 @@ unpackBits ((k,v):xs) r + ----------------------------------------------------------------------------- + -- Address and service lookups + +-#if defined(IPV6_SOCKET_SUPPORT) ++#if defined(IPV6_SOCKET_SUPPORT) || 1 + + -- | Flags that control the querying behaviour of 'getAddrInfo'. + -- For more information, see +@@ -1568,7 +1568,7 @@ foreign import CALLCONV unsafe "bind" + c_bind :: CInt -> Ptr SockAddr -> CInt{-CSockLen???-} -> IO CInt + foreign import CALLCONV SAFE_ON_WIN "connect" + c_connect :: CInt -> Ptr SockAddr -> CInt{-CSockLen???-} -> IO CInt +-#ifdef HAVE_ACCEPT4 ++#if 0 + foreign import CALLCONV unsafe "accept4" + c_accept4 :: CInt -> Ptr SockAddr -> Ptr CInt{-CSockLen???-} -> CInt -> IO CInt + #else +diff --git a/Network/Socket/ByteString.hsc b/Network/Socket/ByteString.hsc +index 93e29c9..a736932 100644 +--- a/Network/Socket/ByteString.hsc ++++ b/Network/Socket/ByteString.hsc +@@ -177,7 +177,7 @@ sendMany sock@(MkSocket fd _ _ _ _) cs = do + liftM fromIntegral . withIOVec cs $ \(iovsPtr, iovsLen) -> + throwSocketErrorWaitWrite sock "writev" $ + c_writev (fromIntegral fd) iovsPtr +- (fromIntegral (min iovsLen (#const IOV_MAX))) ++ (fromIntegral (min iovsLen (0x0026))) + #else + sendMany sock = sendAll sock . B.concat + #endif +diff --git a/Network/Socket/Internal.hsc b/Network/Socket/Internal.hsc +index c8bf4f6..2463bd7 100644 +--- a/Network/Socket/Internal.hsc ++++ b/Network/Socket/Internal.hsc +@@ -24,7 +24,7 @@ module Network.Socket.Internal + ( + -- * Socket addresses + HostAddress +-#if defined(IPV6_SOCKET_SUPPORT) ++#if defined(IPV6_SOCKET_SUPPORTNO) + , HostAddress6 + , FlowInfo + , ScopeID +diff --git a/Network/Socket/Types.hsc b/Network/Socket/Types.hsc +index b42c98d..e5bb9fe 100644 +--- a/Network/Socket/Types.hsc ++++ b/Network/Socket/Types.hsc +@@ -758,10 +758,10 @@ intToPortNumber v = PortNum (htons (fromIntegral v)) + portNumberToInt :: PortNumber -> Int + portNumberToInt (PortNum po) = fromIntegral (ntohs po) + +-foreign import CALLCONV unsafe "ntohs" ntohs :: Word16 -> Word16 +-foreign import CALLCONV unsafe "htons" htons :: Word16 -> Word16 +-foreign import CALLCONV unsafe "ntohl" ntohl :: Word32 -> Word32 +-foreign import CALLCONV unsafe "htonl" htonl :: Word32 -> Word32 ++foreign import CALLCONV unsafe "my_ntohs" ntohs :: Word16 -> Word16 ++foreign import CALLCONV unsafe "my_htons" htons :: Word16 -> Word16 ++foreign import CALLCONV unsafe "my_ntohl" ntohl :: Word32 -> Word32 ++foreign import CALLCONV unsafe "my_htonl" htonl :: Word32 -> Word32 + + instance Enum PortNumber where + toEnum = intToPortNumber +@@ -1071,13 +1071,9 @@ poke32 p i0 a = do + -- | Private newtype proxy for the Storable instance. To avoid orphan instances. + newtype In6Addr = In6Addr HostAddress6 + +-#if __GLASGOW_HASKELL__ < 800 +-#let alignment t = "%lu", (unsigned long)offsetof(struct {char x__; t (y__); }, y__) +-#endif +- + instance Storable In6Addr where + sizeOf _ = #const sizeof(struct in6_addr) +- alignment _ = #alignment struct in6_addr ++ alignment _ = 64 + + peek p = do + a <- peek32 p 0 +diff --git a/cbits/HsNet.c b/cbits/HsNet.c +index 86b55dc..6225c32 100644 +--- a/cbits/HsNet.c ++++ b/cbits/HsNet.c +@@ -6,3 +6,21 @@ + + #define INLINE + #include "HsNet.h" ++ ++#include ++uint16_t my_htons(uint16_t v) ++{ ++ htons(v); ++} ++uint32_t my_htonl(uint32_t v) ++{ ++ htonl(v); ++} ++uint16_t my_ntohs(uint16_t v) ++{ ++ ntohs(v); ++} ++uint32_t my_ntohl(uint32_t v) ++{ ++ ntohl(v); ++} +diff --git a/configure b/configure +index 9e82879..24ef3ce 100755 +--- a/configure ++++ b/configure +@@ -1,4 +1,5 @@ + #! /bin/sh ++set -- --host=arm-linux-androideabi + # Guess values for system-dependent variables and create Makefiles. + # Generated by GNU Autoconf 2.69 for Haskell network package 2.3.0.14. + # +diff --git a/include/HsNetworkConfig.h b/include/HsNetworkConfig.h +index 383f6e2..62b8852 100644 +--- a/include/HsNetworkConfig.h ++++ b/include/HsNetworkConfig.h +@@ -2,7 +2,7 @@ + /* include/HsNetworkConfig.h.in. Generated from configure.ac by autoheader. */ + + /* Define to 1 if you have the `accept4' function. */ +-#define HAVE_ACCEPT4 1 ++/* #undef HAVE_ACCEPT4 */ + + /* Define to 1 if you have the header file. */ + #define HAVE_ARPA_INET_H 1 +@@ -73,7 +73,7 @@ + #define HAVE_LIMITS_H 1 + + /* Define to 1 if you have the header file. */ +-#define HAVE_LINUX_CAN_H 1 ++/* #undef HAVE_LINUX_CAN_H */ + + /* Define to 1 if you have a Linux sendfile(2) implementation. */ + #define HAVE_LINUX_SENDFILE 1 +-- +2.1.4 + diff --git a/standalone/android/haskell-patches/warp_remove-ipv6-stuff.patch b/standalone/android/haskell-patches/warp_remove-ipv6-stuff.patch new file mode 100644 index 0000000000..cc5368f6b4 --- /dev/null +++ b/standalone/android/haskell-patches/warp_remove-ipv6-stuff.patch @@ -0,0 +1,39 @@ +From 2f1d2eddde94d339d91d7b018dc90542b7625fd3 Mon Sep 17 00:00:00 2001 +From: androidbuilder +Date: Wed, 20 Jun 2018 14:41:04 +0100 +Subject: [PATCH] remove ipv6 stuff + +--- + Network/Wai/Handler/Warp/Run.hs | 9 +-------- + 1 file changed, 1 insertion(+), 8 deletions(-) + +diff --git a/Network/Wai/Handler/Warp/Run.hs b/Network/Wai/Handler/Warp/Run.hs +index 116b24e..5c7cbcb 100644 +--- a/Network/Wai/Handler/Warp/Run.hs ++++ b/Network/Wai/Handler/Warp/Run.hs +@@ -14,7 +14,7 @@ import Control.Monad (when, unless, void) + import Data.ByteString (ByteString) + import qualified Data.ByteString as S + import Data.Char (chr) +-import Data.IP (toHostAddress, toHostAddress6) ++import Data.IP (toHostAddress) + import Data.IORef (IORef, newIORef, readIORef, writeIORef) + import Data.Streaming.Network (bindPortTCP) + import Network (sClose, Socket) +@@ -305,13 +305,6 @@ serveConnection conn ii origAddr transport settings app = do + [a] -> Just (SockAddrInet (readInt clientPort) + (toHostAddress a)) + _ -> Nothing +- ["PROXY","TCP6",clientAddr,_,clientPort,_] -> +- case [x | (x, t) <- reads (decodeAscii clientAddr), null t] of +- [a] -> Just (SockAddrInet6 (readInt clientPort) +- 0 +- (toHostAddress6 a) +- 0) +- _ -> Nothing + ("PROXY":"UNKNOWN":_) -> + Just origAddr + _ -> +-- +2.1.4 + diff --git a/standalone/android/haskell-patches/x509-store_support-Android-cert-store.patch b/standalone/android/haskell-patches/x509-store_support-Android-cert-store.patch new file mode 100644 index 0000000000..1683f49745 --- /dev/null +++ b/standalone/android/haskell-patches/x509-store_support-Android-cert-store.patch @@ -0,0 +1,28 @@ +From 717945172c2f3ff95cce9db2d075122bccfc9a1a Mon Sep 17 00:00:00 2001 +From: androidbuilder +Date: Wed, 20 Jun 2018 02:01:30 +0100 +Subject: [PATCH] support Android cert store + +Android has only hashsed cert files. +See https://github.com/vincenthz/hs-certificate/issues/19 +--- + Data/X509/CertificateStore.hs | 2 +- + 2 files changed, 1 insertion(+), 1 deletion(-) + delete mode 100644 Data/X509/.CertificateStore.hs.swp + +diff --git a/Data/X509/CertificateStore.hs b/Data/X509/CertificateStore.hs +index 07449a2..74b8bde 100644 +--- a/Data/X509/CertificateStore.hs ++++ b/Data/X509/CertificateStore.hs +@@ -106,7 +106,7 @@ listDirectoryCerts path = + && isDigit (s !! 9) + && (s !! 8) == '.' + && all isHexDigit (take 8 s) +- isCert x = (not $ isPrefixOf "." x) && (not $ isHashedFile x) ++ isCert x = (not $ isPrefixOf "." x) + + getDirContents = E.catch (map (path ) . filter isCert <$> getDirectoryContents path) emptyPaths + where emptyPaths :: E.IOException -> IO [FilePath] +-- +2.1.4 + diff --git a/standalone/android/haskell-patches/x509-system_support-Android-cert-store.patch b/standalone/android/haskell-patches/x509-system_support-Android-cert-store.patch deleted file mode 100644 index 14ed66089d..0000000000 --- a/standalone/android/haskell-patches/x509-system_support-Android-cert-store.patch +++ /dev/null @@ -1,27 +0,0 @@ -From 61d0e47cd038f25157e48385fc080d0d374b214d Mon Sep 17 00:00:00 2001 -From: dummy -Date: Tue, 14 Oct 2014 02:07:57 +0000 -Subject: [PATCH] support Android cert store - -Android has only hashsed cert files. -See https://github.com/vincenthz/hs-certificate/issues/19 ---- - System/X509/Unix.hs | 2 +- - 1 file changed, 1 insertion(+), 1 deletion(-) - -diff --git a/System/X509/Unix.hs b/System/X509/Unix.hs -index 9df3331..a30da26 100644 ---- a/System/X509/Unix.hs -+++ b/System/X509/Unix.hs -@@ -56,7 +56,7 @@ listDirectoryCerts path = do - && isDigit (s !! 9) - && (s !! 8) == '.' - && all isHexDigit (take 8 s) -- isCert x = (not $ isPrefixOf "." x) && (not $ isHashedFile x) -+ isCert x = (not $ isPrefixOf "." x) - - getDirContents = E.catch (Just <$> getDirectoryContents path) emptyPaths - where emptyPaths :: E.IOException -> IO (Maybe [FilePath]) --- -1.7.10.4 - diff --git a/standalone/android/haskell-patches/xss-sanitize_deps.patch b/standalone/android/haskell-patches/xss-sanitize_deps.patch new file mode 100644 index 0000000000..0f0cfd9256 --- /dev/null +++ b/standalone/android/haskell-patches/xss-sanitize_deps.patch @@ -0,0 +1,24 @@ +From 41eb8ab50125eb6ccf260c5510407483f1d78dd4 Mon Sep 17 00:00:00 2001 +From: dummy +Date: Wed, 20 Jun 2018 14:52:52 +0100 +Subject: [PATCH] deps + +--- + xss-sanitize.cabal | 1 + + 1 file changed, 1 insertion(+) + +diff --git a/xss-sanitize.cabal b/xss-sanitize.cabal +index 727dc95..2de4270 100644 +--- a/xss-sanitize.cabal ++++ b/xss-sanitize.cabal +@@ -19,6 +19,7 @@ library + , tagsoup >= 0.12.2 && < 1 + , utf8-string >= 0.3 && < 1 + , network >= 2 && < 3 ++ , network-uri + , css-text >= 0.1.1 && < 0.2 + , text >= 0.11 && < 2 + , attoparsec >= 0.10.0.3 && < 1 +-- +2.1.4 + diff --git a/standalone/android/install-haskell-packages b/standalone/android/install-haskell-packages index 7d79d0b801..db6cf621a2 100755 --- a/standalone/android/install-haskell-packages +++ b/standalone/android/install-haskell-packages @@ -80,7 +80,6 @@ EOF patched unix-time patched lifted-base patched zlib - patched MissingH patched distributive patched comonad patched iproute @@ -93,15 +92,17 @@ EOF patched skein patched lens patched certificate - patched x509-system + patched x509-store patched persistent-template patched system-filepath patched optparse-applicative + patched warp patched wai-app-static patched yesod-routes patched shakespeare patched yesod-core patched yesod-persistent + patched xss-sanitize patched yesod-form patched crypto-numbers patched clock @@ -113,6 +114,7 @@ EOF patched dns patched unbounded-delays patched uuid + patched basement cd .. From 537935333f58b4120405bb15a614213fb237d72e Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 11:27:56 -0400 Subject: [PATCH 24/41] document CVE-2018-10859 --- ...hole_private_data_exposure_via_addurl.mdwn | 4 + .../CVE-2018-10857_and_CVE-2018-10859.mdwn | 101 ++++++++++++------ 2 files changed, 73 insertions(+), 32 deletions(-) diff --git a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn index f1884d8f31..ae4f6f9857 100644 --- a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn +++ b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn @@ -143,6 +143,7 @@ So if the attacker knows a file that the user has encrypted with any of their gpg keys, they can provide that file, and hope it will be decrypted. Note that this does not need a redirect to a local file or web server; the attacker can make their web server serve up a gpg encrypted file. +This is a separate vulnerability and was assigned CVE-2018-10859. So, content downloaded from encrypted special remotes (both internal and external) must be rejected unless it can be verified with a hash. Then @@ -154,6 +155,9 @@ untrusted third parties, and relax that check. > TODO +Tocho Tochev has updated git-annex-remote-pcloud to not follow http +redirects. + ---- Built-in special remotes that use protocols on top of http, eg S3 and WebDAV, diff --git a/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn b/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn index 88ec3d36d2..a5bf747529 100644 --- a/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn +++ b/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn @@ -3,52 +3,89 @@ exposure and exfiltration attack. It could expose the content of files located outside the git-annex repository, or content from a private web server on localhost or the LAN. Joey Hess discovered this attack. -Additionally, git-annex encrypted special remotes could be leveraged -by an attacker to decrypt files that were encrypted to the user's gpg +CVE-2018-10859: A malicious server for a special remote could +trick git-annex into decrypting a file that was encrypted to the user's gpg key. This attack could be used to expose encrypted data that was never stored in git-annex. Daniel Dent discovered this attack in collaboration with Joey Hess. -This was fixed in git-annex 6.20180622. +These security holes were fixed in git-annex 6.20180622. -## details +Also, some external special remotes (plugins) were improved, as a second +line of defense against CVE-2018-10857: -The attacker needs to have control over one of the remotes of the git-annex -repository. For example, they may provide a public git-annex repository that -the victim clones. Or the victim may have paired repositories with them. Or, +* git-annex-remote-pcloud 0.0.2 (thanks to Tocho Tochev) + +## attack descriptions + +To perform these attacks, the attacker needs to have control over one of +the remotes of the victim's git-annex repository. For example, they may +provide a public git-annex repository that the victim clones. Or, equivilantly, the attacker could have read access to the victim's git-annex -repository (eg on a server somewhere), and some channel to get commits into it -(eg a pull requests). +repository or a repository it pushes to, and some channel to get commits +into it (eg pull requests). -To perform the private data and exfiltration attack, the attacker -runs `git-annex addurl --relaxed file:///etc/passwd` and commits this to -the repository in some out of the way place. Then they wait for the victim -to pull the change. (As well as `file:///` urls, the attacker can use urls -to private web servers. The url can also be one that the attacker controls, -that redirects to such urls.) +These exploits are most likely to succeed when the victim is running the +git-annex assistant, or is periodically running `git annex sync --content`. -To perform the gpg decryption attack, the attacker also needs to have -control of an encrypted special remote of the victim's git-annex -repository. The attacker uses `git annex addurl --relaxed` with -an innocuous url, and waits for the user's git-annex to download it, -and upload an (encrypted) copy to the special remote they also control. -At a later point, when the user downloads the content from the special -remote, the attacker instead sends them the content of a gpg encrypted -file they wish to have decrypted in its place. Finally, the attacker -drops their own copy of the original innocuous url, and waits for the user -to send them the decrypted form of the file they earlier sent. +To perform the private data exfiltration attack (CVE-2018-10857), the +attacker runs `git-annex addurl --relaxed file:///etc/passwd` and commits +this to the repository in some out of the way place. After the victim's +git repository receives that change, git-annex follows the attacker-provided +url to private data, which it stores in the git-annex repository. +From there it transfers the content to the git-annex repository that +the attacker has access to. -The easiest exploit is when the victim is running the git-annex assistant, or -is periodically doing `git annex sync --content`. The victim may also perform -the equivilant actions manually, not noticing they're operating on the file. +(As well as `file:///` urls, the attacker can use urls to private web +servers. The url can also be one that the attacker controls, that redirects +to such urls.) -In either case, git-annex gets the content of the annexed file, following -the attacker-provider url to private data. Then git-annex transfers the content -back to the attacker's repository. +To perform the gpg decryption attack (CVE-2018-10859), the attacker +additionally needs to have control of the server hosting an encrypted +special remote used by the victim's git-annex repository. The attacker uses +`git annex addurl --relaxed` with an innocuous url, and waits for the +user's git-annex to download it, and upload an (encrypted) copy to the +special remote they also control. At some later point, when the user +downloads the content from the special remote, the attacker instead sends +them the content of a gpg encrypted file that they wish to have decrypted +in its place. Finally, the attacker drops their own copy of the original +innocuous url, and waits for git-annex to send them the accidentially +decrypted file. -This attack was fixed by making git-annex refuse to follow `file:///` urls +## git-annex security fixes + +CVE-2018-10857 was fixed by making git-annex refuse to follow `file:///` urls and urls pointing to private/local IP addresses by default. Two new configuration settings, annex.security.allowed-url-schemes and annex.security.allowed-http-addresses, can relax this security policy, and are intended for cases where the git-annex repository is kept private and so the attack does not apply. + +CVE-2018-10859 was fixed by making git-annex refuse to download encrypted +content from special remotes, unless it knows the hash of the expected +content. When the attacker provides some other gpg encrypted content, it +will fail the hash check and be discarded. + +External special remotes (plugins) that use HTTP/HTTPS could also be +attacked using the CVE-2018-10857 method, if the attacker additionally has +control of the server they connect to. To prevent such attacks, +git-annex refuses to download content from external special remotes unless +it can verify the hash of that content. + +## impact on external special remotes + +One variant of CVE-2018-10857 can exploit a vulnerable external special +remote, and could not be prevented by git-annex. (git-annex's own built-in +special remotes are not vulnerable to this attack.) + +In this attack variant, the attacker guesses at the hash of a file stored +on the victim's private web server, and adds it to the git-annex +repository. The attacker also has control of the server hosting an +encrypted special remote used by the victim's git-annex repository. They +cause that server to redirect to the victim's web server. This allows the +attacker to verify if the victim's web server contains a file that the +attacker already knows the content of, assuming they can guess the URL to +it. + +Developers of external special remotes are encouraged to prevent this +attack by not following such HTTP redirects. From 4315bb9e421f2c643e517d8982c6c35b1909c78b Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 11:35:27 -0400 Subject: [PATCH 25/41] add retrievalSecurityPolicy This will be used to protect against CVE-2018-10859, where an encrypted special remote is fed the wrong encrypted data, and so tricked into decrypting something that the user encrypted with their gpg key and did not store in git-annex. It also protects against CVE-2018-10857, where a remote follows a http redirect to a file:// url or to a local private web server. While that's already been prevented in git-annex's own use of http, external special remotes, hooks, etc use other http implementations and could still be vulnerable. The policy is not yet enforced, this commit only adds the appropriate metadata to remotes. This commit was sponsored by Boyd Stephen Smith Jr. on Patreon. --- Remote/Adb.hs | 1 + Remote/BitTorrent.hs | 2 ++ Remote/Bup.hs | 3 +++ Remote/Ddar.hs | 2 ++ Remote/Directory.hs | 1 + Remote/External.hs | 5 +++++ Remote/GCrypt.hs | 1 + Remote/Git.hs | 1 + Remote/Glacier.hs | 3 +++ Remote/Helper/Special.hs | 8 ++++++++ Remote/Hook.hs | 3 +++ Remote/P2P.hs | 1 + Remote/Rsync.hs | 1 + Remote/S3.hs | 3 +++ Remote/Tahoe.hs | 2 ++ Remote/Web.hs | 3 +++ Remote/WebDAV.hs | 3 +++ Types/Key.hs | 19 ++++++++++++++++++- Types/Remote.hs | 31 ++++++++++++++++++++++++++++++- 19 files changed, 91 insertions(+), 2 deletions(-) diff --git a/Remote/Adb.hs b/Remote/Adb.hs index f1e87013de..8e99f1f749 100644 --- a/Remote/Adb.hs +++ b/Remote/Adb.hs @@ -48,6 +48,7 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = \_ _ _ -> return False + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/BitTorrent.hs b/Remote/BitTorrent.hs index 8cc559d917..659cc4732a 100644 --- a/Remote/BitTorrent.hs +++ b/Remote/BitTorrent.hs @@ -59,6 +59,8 @@ gen r _ c gc = , storeKey = uploadKey , retrieveKeyFile = downloadKey , retrieveKeyFileCheap = downloadKeyCheap + -- Bittorrent does its own hash checks. + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = dropKey , lockContent = Nothing , checkPresent = checkKey diff --git a/Remote/Bup.hs b/Remote/Bup.hs index 7b9ed4b7da..024e06a1b5 100644 --- a/Remote/Bup.hs +++ b/Remote/Bup.hs @@ -59,6 +59,9 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap buprepo + -- Bup uses git, which cryptographically verifies content + -- (with SHA1, but sufficiently for this). + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/Ddar.hs b/Remote/Ddar.hs index c37abde82c..1ca1fda159 100644 --- a/Remote/Ddar.hs +++ b/Remote/Ddar.hs @@ -58,6 +58,8 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap + -- Unsure about this, safe default until Robie answers. + , retrievalSecurityPolicy = RetrievalVerifiableKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/Directory.hs b/Remote/Directory.hs index dd79ea04af..2fcb05d93a 100644 --- a/Remote/Directory.hs +++ b/Remote/Directory.hs @@ -58,6 +58,7 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveKeyFileCheapM dir chunkconfig + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/External.hs b/Remote/External.hs index dada0d9d81..1427d61456 100644 --- a/Remote/External.hs +++ b/Remote/External.hs @@ -109,6 +109,11 @@ gen r u c gc , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = \_ _ _ -> return False + -- External special remotes use many http libraries + -- and have no protection against redirects to + -- local private web servers, or in some cases + -- to file:// urls. + , retrievalSecurityPolicy = RetrievalVerifiableKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/GCrypt.hs b/Remote/GCrypt.hs index 37b708579e..20c47334d4 100644 --- a/Remote/GCrypt.hs +++ b/Remote/GCrypt.hs @@ -113,6 +113,7 @@ gen' r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = \_ _ _ -> return False + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/Git.hs b/Remote/Git.hs index da177bac58..3f85365acf 100644 --- a/Remote/Git.hs +++ b/Remote/Git.hs @@ -161,6 +161,7 @@ gen r u c gc , storeKey = copyToRemote new st , retrieveKeyFile = copyFromRemote new st , retrieveKeyFileCheap = copyFromRemoteCheap new st + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = dropKey new st , lockContent = Just (lockKey new st) , checkPresent = inAnnex new st diff --git a/Remote/Glacier.hs b/Remote/Glacier.hs index d5f66172ae..e6cd68fdf3 100644 --- a/Remote/Glacier.hs +++ b/Remote/Glacier.hs @@ -55,6 +55,9 @@ gen r u c gc = new <$> remoteCost gc veryExpensiveRemoteCost , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap this + -- glacier-cli does not follow redirects and does + -- not support file://, so this is secure. + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/Helper/Special.hs b/Remote/Helper/Special.hs index 73486442b8..883dcc9cb1 100644 --- a/Remote/Helper/Special.hs +++ b/Remote/Helper/Special.hs @@ -162,6 +162,14 @@ specialRemote' cfg c preparestorer prepareretriever prepareremover preparecheckp (retrieveKeyFileCheap baser k f d) -- retrieval of encrypted keys is never cheap (\_ -> return False) + -- When encryption is used, the remote could provide + -- some other content encrypted by the user, and trick + -- git-annex into decrypting it, leaking the decryption + -- into the git-annex repository. Verifiable keys + -- are the main protection against this attack. + , retrievalSecurityPolicy = if isencrypted + then RetrievalVerifiableKeysSecure + else retrievalSecurityPolicy baser , removeKey = \k -> cip >>= removeKeyGen k , checkPresent = \k -> cip >>= checkPresentGen k , cost = if isencrypted diff --git a/Remote/Hook.hs b/Remote/Hook.hs index 54d0480145..a6e5339732 100644 --- a/Remote/Hook.hs +++ b/Remote/Hook.hs @@ -49,6 +49,9 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap hooktype + -- A hook could use http and be vulnerable to + -- redirect to file:// attacks, etc. + , retrievalSecurityPolicy = RetrievalVerifiableKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/P2P.hs b/Remote/P2P.hs index b5ff048639..c47a9e6327 100644 --- a/Remote/P2P.hs +++ b/Remote/P2P.hs @@ -53,6 +53,7 @@ chainGen addr r u c gc = do , storeKey = store (const protorunner) , retrieveKeyFile = retrieve (const protorunner) , retrieveKeyFileCheap = \_ _ _ -> return False + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = remove protorunner , lockContent = Just $ lock withconn runProtoConn u , checkPresent = checkpresent protorunner diff --git a/Remote/Rsync.hs b/Remote/Rsync.hs index 9ce9c7f148..8f4e8ac55a 100644 --- a/Remote/Rsync.hs +++ b/Remote/Rsync.hs @@ -72,6 +72,7 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap o + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/S3.hs b/Remote/S3.hs index 227d0c5e85..5665455809 100644 --- a/Remote/S3.hs +++ b/Remote/S3.hs @@ -84,6 +84,9 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap + -- HttpManagerRestricted is used here, so this is + -- secure. + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Remote/Tahoe.hs b/Remote/Tahoe.hs index ae3d6950bd..5615c4849d 100644 --- a/Remote/Tahoe.hs +++ b/Remote/Tahoe.hs @@ -73,6 +73,8 @@ gen r u c gc = do , storeKey = store u hdl , retrieveKeyFile = retrieve u hdl , retrieveKeyFileCheap = \_ _ _ -> return False + -- Tahoe cryptographically verifies content. + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = remove , lockContent = Nothing , checkPresent = checkKey u hdl diff --git a/Remote/Web.hs b/Remote/Web.hs index 03cbe706d5..eef6b696e6 100644 --- a/Remote/Web.hs +++ b/Remote/Web.hs @@ -48,6 +48,9 @@ gen r _ c gc = , storeKey = uploadKey , retrieveKeyFile = downloadKey , retrieveKeyFileCheap = downloadKeyCheap + -- HttpManagerRestricted is used here, so this is + -- secure. + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = dropKey , lockContent = Nothing , checkPresent = checkKey diff --git a/Remote/WebDAV.hs b/Remote/WebDAV.hs index 432b729ca0..d8fc8bee4f 100644 --- a/Remote/WebDAV.hs +++ b/Remote/WebDAV.hs @@ -72,6 +72,9 @@ gen r u c gc = new <$> remoteCost gc expensiveRemoteCost , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap + -- HttpManagerRestricted is used here, so this is + -- secure. + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy diff --git a/Types/Key.hs b/Types/Key.hs index b3efc04633..4b81850d80 100644 --- a/Types/Key.hs +++ b/Types/Key.hs @@ -1,6 +1,6 @@ {- git-annex Key data type - - - Copyright 2011-2017 Joey Hess + - Copyright 2011-2018 Joey Hess - - Licensed under the GNU GPL version 3 or higher. -} @@ -88,6 +88,23 @@ cryptographicallySecure (Blake2sKey _ _) = True cryptographicallySecure (Blake2spKey _ _) = True cryptographicallySecure _ = False +{- Is the Key variety backed by a hash, which allows verifying content? + - It does not have to be cryptographically secure against eg birthday + - attacks. + -} +isVerifiable :: KeyVariety -> Bool +isVerifiable (SHA2Key _ _) = True +isVerifiable (SHA3Key _ _) = True +isVerifiable (SKEINKey _ _) = True +isVerifiable (Blake2bKey _ _) = True +isVerifiable (Blake2sKey _ _) = True +isVerifiable (Blake2spKey _ _) = True +isVerifiable (SHA1Key _) = True +isVerifiable (MD5Key _) = True +isVerifiable WORMKey = False +isVerifiable URLKey = False +isVerifiable (OtherKey _) = False + formatKeyVariety :: KeyVariety -> String formatKeyVariety v = case v of SHA2Key sz e -> adde e (addsz sz "SHA") diff --git a/Types/Remote.hs b/Types/Remote.hs index f50bcef693..9f61f7041d 100644 --- a/Types/Remote.hs +++ b/Types/Remote.hs @@ -2,7 +2,7 @@ - - Most things should not need this, using Types instead - - - Copyright 2011-2017 Joey Hess + - Copyright 2011-2018 Joey Hess - - Licensed under the GNU GPL version 3 or higher. -} @@ -18,6 +18,7 @@ module Types.Remote , Availability(..) , Verification(..) , unVerified + , RetrievalSecurityPolicy(..) , isExportSupported , ExportActions(..) ) @@ -85,6 +86,8 @@ data RemoteA a = Remote -- Retrieves a key's contents to a tmp file, if it can be done cheaply. -- It's ok to create a symlink or hardlink. , retrieveKeyFileCheap :: Key -> AssociatedFile -> FilePath -> a Bool + -- Security policy for reteiving keys from this remote. + , retrievalSecurityPolicy :: RetrievalSecurityPolicy -- Removes a key's contents (succeeds if the contents are not present) , removeKey :: Key -> a Bool -- Uses locking to prevent removal of a key's contents, @@ -165,6 +168,32 @@ unVerified a = do ok <- a return (ok, UnVerified) +-- Security policy indicating what keys can be safely retrieved from a +-- remote. +data RetrievalSecurityPolicy + = RetrievalVerifiableKeysSecure + -- ^ Transfer of keys whose content can be verified + -- with a hash check is secure; transfer of unverifiable keys is + -- not secure and should not be allowed. + -- + -- This is used eg, when HTTP to a remote could be redirected to a + -- local private web server or even a file:// url, causing private + -- data from it that is not the intended content of a key to make + -- its way into the git-annex repository. + -- + -- It's also used when content is stored encrypted on a remote, + -- which could replace it with a different encrypted file, and + -- trick git-annex into decrypting it and leaking the decryption + -- into the git-annex repository. + -- + -- It's not (currently) used when the remote could alter the + -- content stored on it, because git-annex does not provide + -- strong guarantees about the content of keys that cannot be + -- verified with a hash check. + -- (But annex.securehashesonly does provide such guarantees.) + | RetrievalAllKeysSecure + -- ^ Any key can be securely retrieved. + isExportSupported :: RemoteA a -> a Bool isExportSupported r = exportSupported (remotetype r) (config r) (gitconfig r) From c981683f77253b2fa4e3f85d5593e6b1944d7746 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 12:28:39 -0400 Subject: [PATCH 26/41] date deferred devblogs --- doc/devblog/day_502__security_hole_part_4.mdwn | 2 ++ doc/devblog/day_503__security_hole_part_5.mdwn | 2 ++ 2 files changed, 4 insertions(+) diff --git a/doc/devblog/day_502__security_hole_part_4.mdwn b/doc/devblog/day_502__security_hole_part_4.mdwn index 43b66300b3..8988a7f15d 100644 --- a/doc/devblog/day_502__security_hole_part_4.mdwn +++ b/doc/devblog/day_502__security_hole_part_4.mdwn @@ -20,3 +20,5 @@ Debian stable, which will probably be more palatable to their security team than the full 2000+ lines of patches I've developed so far. The minimal fix is secure, but suboptimal; it prevents even safe urls from being downloaded from the web special remote by default. + +[[!meta date="June 18 2018 4:00 pm"]] diff --git a/doc/devblog/day_503__security_hole_part_5.mdwn b/doc/devblog/day_503__security_hole_part_5.mdwn index 88f800e12f..86c760c6c2 100644 --- a/doc/devblog/day_503__security_hole_part_5.mdwn +++ b/doc/devblog/day_503__security_hole_part_5.mdwn @@ -15,3 +15,5 @@ pull off, but it's still super bad. This week is going to be a lot longer than I thought, and it's already feeling kind of endless.. + +[[!meta date="June 19 2018 8:00 pm"]] From b657242f5d946efae4cc77e8aef95dd2a306cd6b Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 13:34:11 -0400 Subject: [PATCH 27/41] enforce retrievalSecurityPolicy Leveraged the existing verification code by making it also check the retrievalSecurityPolicy. Also, prevented getViaTmp from running the download action at all when the retrievalSecurityPolicy is going to prevent verifying and so storing it. Added annex.security.allow-unverified-downloads. A per-remote version would be nice to have too, but would need more plumbing, so KISS. (Bill the Cat reference not too over the top I hope. The point is to make this something the user reads the documentation for before using.) A few calls to verifyKeyContent and getViaTmp, that don't involve downloads from remotes, have RetrievalAllKeysSecure hard-coded. It was also hard-coded for P2P.Annex and Command.RecvKey, to match the values of the corresponding remotes. A few things use retrieveKeyFile/retrieveKeyFileCheap without going through getViaTmp. * Command.Fsck when downloading content from a remote to verify it. That content does not get into the annex, so this is ok. * Command.AddUrl when using a remote to download an url; this is new content being added, so this is ok. This commit was sponsored by Fernando Jimenez on Patreon. --- Annex/Content.hs | 65 +++++++++++++++++++++++++++++++---------- CHANGELOG | 13 +++++++-- Command/Get.hs | 2 +- Command/Move.hs | 2 +- Command/Multicast.hs | 2 +- Command/ReKey.hs | 2 +- Command/RecvKey.hs | 5 +++- Command/Reinject.hs | 2 +- Command/SetKey.hs | 2 +- Command/TestRemote.hs | 8 ++--- Command/TransferKey.hs | 2 +- Command/TransferKeys.hs | 2 +- NEWS | 6 ++++ P2P/Annex.hs | 6 +++- Remote.hs | 1 + Remote/Git.hs | 3 +- Types/GitConfig.hs | 3 ++ doc/git-annex.mdwn | 39 ++++++++++++++++++++++++- 18 files changed, 131 insertions(+), 34 deletions(-) diff --git a/Annex/Content.hs b/Annex/Content.hs index 9f3722c609..8902114dad 100644 --- a/Annex/Content.hs +++ b/Annex/Content.hs @@ -1,6 +1,6 @@ {- git-annex file content managing - - - Copyright 2010-2017 Joey Hess + - Copyright 2010-2018 Joey Hess - - Licensed under the GNU GPL version 3 or higher. -} @@ -15,6 +15,7 @@ module Annex.Content ( lockContentShared, lockContentForRemoval, ContentRemovalLock, + RetrievalSecurityPolicy(..), getViaTmp, getViaTmpFromDisk, checkDiskSpaceToGet, @@ -78,7 +79,7 @@ import qualified Annex.Content.Direct as Direct import Annex.ReplaceFile import Annex.LockPool import Messages.Progress -import Types.Remote (unVerified, Verification(..)) +import Types.Remote (unVerified, Verification(..), RetrievalSecurityPolicy(..)) import qualified Types.Remote import qualified Types.Backend import qualified Backend @@ -293,15 +294,15 @@ lockContentUsing locker key a = do {- Runs an action, passing it the temp file to get, - and if the action succeeds, verifies the file matches - the key and moves the file into the annex as a key's content. -} -getViaTmp :: VerifyConfig -> Key -> (FilePath -> Annex (Bool, Verification)) -> Annex Bool -getViaTmp v key action = checkDiskSpaceToGet key False $ - getViaTmpFromDisk v key action +getViaTmp :: RetrievalSecurityPolicy -> VerifyConfig -> Key -> (FilePath -> Annex (Bool, Verification)) -> Annex Bool +getViaTmp rsp v key action = checkDiskSpaceToGet key False $ + getViaTmpFromDisk rsp v key action {- Like getViaTmp, but does not check that there is enough disk space - for the incoming key. For use when the key content is already on disk - and not being copied into place. -} -getViaTmpFromDisk :: VerifyConfig -> Key -> (FilePath -> Annex (Bool, Verification)) -> Annex Bool -getViaTmpFromDisk v key action = do +getViaTmpFromDisk :: RetrievalSecurityPolicy -> VerifyConfig -> Key -> (FilePath -> Annex (Bool, Verification)) -> Annex Bool +getViaTmpFromDisk rsp v key action = checkallowed $ do tmpfile <- prepTmp key resuming <- liftIO $ doesFileExist tmpfile (ok, verification) <- action tmpfile @@ -315,7 +316,7 @@ getViaTmpFromDisk v key action = do _ -> MustVerify else verification if ok - then ifM (verifyKeyContent v verification' key tmpfile) + then ifM (verifyKeyContent rsp v verification' key tmpfile) ( ifM (pruneTmpWorkDirBefore tmpfile (moveAnnex key)) ( do logStatus key InfoPresent @@ -330,24 +331,46 @@ getViaTmpFromDisk v key action = do -- On transfer failure, the tmp file is left behind, in case -- caller wants to resume its transfer else return False + where + -- Avoid running the action to get the content when the + -- RetrievalSecurityPolicy would cause verification to always fail. + checkallowed a = case rsp of + RetrievalAllKeysSecure -> a + RetrievalVerifiableKeysSecure + | isVerifiable (keyVariety key) -> a + | otherwise -> ifM (annexAllowUnverifiedDownloads <$> Annex.getGitConfig) + ( a + , warnUnverifiableInsecure key >> return False + ) {- Verifies that a file is the expected content of a key. + - - Configuration can prevent verification, for either a - - particular remote or always. + - particular remote or always, unless the RetrievalSecurityPolicy + - requires verification. - - Most keys have a known size, and if so, the file size is checked. - - - When the key's backend allows verifying the content (eg via checksum), + - When the key's backend allows verifying the content (via checksum), - it is checked. + - + - If the RetrievalSecurityPolicy requires verification and the key's + - backend doesn't support it, the verification will fail. -} -verifyKeyContent :: VerifyConfig -> Verification -> Key -> FilePath -> Annex Bool -verifyKeyContent v verification k f = case verification of - Verified -> return True - UnVerified -> ifM (shouldVerify v) +verifyKeyContent :: RetrievalSecurityPolicy -> VerifyConfig -> Verification -> Key -> FilePath -> Annex Bool +verifyKeyContent rsp v verification k f = case (rsp, verification) of + (_, Verified) -> return True + (RetrievalVerifiableKeysSecure, _) + | isVerifiable (keyVariety k) -> verify + | otherwise -> ifM (annexAllowUnverifiedDownloads <$> Annex.getGitConfig) + ( verify + , warnUnverifiableInsecure k >> return False + ) + (_, UnVerified) -> ifM (shouldVerify v) ( verify , return True ) - MustVerify -> verify + (_, MustVerify) -> verify where verify = verifysize <&&> verifycontent verifysize = case keySize k of @@ -359,6 +382,16 @@ verifyKeyContent v verification k f = case verification of Nothing -> return True Just verifier -> verifier k f +warnUnverifiableInsecure :: Key -> Annex () +warnUnverifiableInsecure k = warning $ unwords + [ "Getting " ++ kv ++ " keys with this remote is not secure;" + , "the content cannot be verified to be correct." + , "(Use annex.security.allow-unverified-downloads to bypass" + , "this safety check.)" + ] + where + kv = formatKeyVariety (keyVariety k) + data VerifyConfig = AlwaysVerify | NoVerify | RemoteVerify Remote | DefaultVerify shouldVerify :: VerifyConfig -> Annex Bool @@ -827,7 +860,7 @@ isUnmodified key f = go =<< geti go (Just fc) = cheapcheck fc <||> expensivecheck fc cheapcheck fc = anyM (compareInodeCaches fc) =<< Database.Keys.getInodeCaches key - expensivecheck fc = ifM (verifyKeyContent AlwaysVerify UnVerified key f) + expensivecheck fc = ifM (verifyKeyContent RetrievalAllKeysSecure AlwaysVerify UnVerified key f) -- The file could have been modified while it was -- being verified. Detect that. ( geti >>= maybe (return False) (compareInodeCaches fc) diff --git a/CHANGELOG b/CHANGELOG index b80205f9af..6f82a31b76 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,10 +1,19 @@ git-annex (6.20180622) upstream; urgency=high - Security fix release for CVE-2018-10857 + Security fix release for CVE-2018-10857 and CVE-2018-10859 + * Refuse to download content, that cannot be verified with a hash, + from encrypted special remotes (for CVE-2018-10859), + and from all external special remotes (for CVE-2018-10857). + In particular, URL and WORM keys stored on such remotes won't + be downloaded. If this affects your files, you can run + `git-annex migrate` on the affected files, to convert them + to use a hash. + * Added annex.security.allow-unverified-downloads, which can override + the above. * Added annex.security.allowed-url-schemes setting, which defaults to only allowing http, https, and ftp URLs. Note especially that file:/ - is no longer enabled by default. This is a security fix. + is no longer enabled by default. * Removed annex.web-download-command, since its interface does not allow supporting annex.security.allowed-url-schemes across redirects. If you used this setting, you may want to instead use annex.web-options diff --git a/Command/Get.hs b/Command/Get.hs index f4e3d47b58..eac8e88a44 100644 --- a/Command/Get.hs +++ b/Command/Get.hs @@ -109,7 +109,7 @@ getKey' key afile = dispatch | Remote.hasKeyCheap r = either (const False) id <$> Remote.hasKey r key | otherwise = return True - docopy r witness = getViaTmp (RemoteVerify r) key $ \dest -> + docopy r witness = getViaTmp (Remote.retrievalSecurityPolicy r) (RemoteVerify r) key $ \dest -> download (Remote.uuid r) key afile stdRetry (\p -> do showAction $ "from " ++ Remote.name r diff --git a/Command/Move.hs b/Command/Move.hs index 2f3079207e..b50c877bcc 100644 --- a/Command/Move.hs +++ b/Command/Move.hs @@ -207,7 +207,7 @@ fromPerform src removewhen key afile = do where go = notifyTransfer Download afile $ download (Remote.uuid src) key afile stdRetry $ \p -> - getViaTmp (RemoteVerify src) key $ \t -> + getViaTmp (Remote.retrievalSecurityPolicy src) (RemoteVerify src) key $ \t -> Remote.retrieveKeyFile src key afile t p dispatch _ _ False = stop -- failed dispatch RemoveNever _ True = next $ return True -- copy complete diff --git a/Command/Multicast.hs b/Command/Multicast.hs index e2f6870c6b..5c853ddd92 100644 --- a/Command/Multicast.hs +++ b/Command/Multicast.hs @@ -213,7 +213,7 @@ storeReceived f = do warning $ "Received a file " ++ f ++ " that is not a git-annex key. Deleting this file." liftIO $ nukeFile f Just k -> void $ - getViaTmpFromDisk AlwaysVerify k $ \dest -> unVerified $ + getViaTmpFromDisk RetrievalVerifiableKeysSecure AlwaysVerify k $ \dest -> unVerified $ liftIO $ catchBoolIO $ do rename f dest return True diff --git a/Command/ReKey.hs b/Command/ReKey.hs index be67a25ab3..4de6e966d1 100644 --- a/Command/ReKey.hs +++ b/Command/ReKey.hs @@ -83,7 +83,7 @@ linkKey file oldkey newkey = ifM (isJust <$> isAnnexLink file) - This avoids hard linking to content linked to an - unlocked file, which would leave the new key unlocked - and vulnerable to corruption. -} - ( getViaTmpFromDisk DefaultVerify newkey $ \tmp -> unVerified $ do + ( getViaTmpFromDisk RetrievalAllKeysSecure DefaultVerify newkey $ \tmp -> unVerified $ do oldobj <- calcRepo (gitAnnexLocation oldkey) linkOrCopy' (return True) newkey oldobj tmp Nothing , do diff --git a/Command/RecvKey.hs b/Command/RecvKey.hs index 103db559b8..84f71f8835 100644 --- a/Command/RecvKey.hs +++ b/Command/RecvKey.hs @@ -13,6 +13,7 @@ import Annex.Action import Annex import Utility.Rsync import Types.Transfer +import Types.Remote (RetrievalSecurityPolicy(..)) import Command.SendKey (fieldTransfer) import qualified CmdLine.GitAnnexShell.Fields as Fields @@ -31,7 +32,9 @@ start key = fieldTransfer Download key $ \_p -> do fromunlocked <- (isJust <$> Fields.getField Fields.unlocked) <||> (isJust <$> Fields.getField Fields.direct) let verify = if fromunlocked then AlwaysVerify else DefaultVerify - ifM (getViaTmp verify key go) + -- This matches the retrievalSecurityPolicy of Remote.Git + let rsp = RetrievalAllKeysSecure + ifM (getViaTmp rsp verify key go) ( do -- forcibly quit after receiving one key, -- and shutdown cleanly diff --git a/Command/Reinject.hs b/Command/Reinject.hs index 48f50d3241..bde4c81ea1 100644 --- a/Command/Reinject.hs +++ b/Command/Reinject.hs @@ -45,7 +45,7 @@ startSrcDest (src:dest:[]) showStart "reinject" dest next $ ifAnnexed dest go stop where - go key = ifM (verifyKeyContent DefaultVerify UnVerified key src) + go key = ifM (verifyKeyContent RetrievalAllKeysSecure DefaultVerify UnVerified key src) ( perform src key , error "failed" ) diff --git a/Command/SetKey.hs b/Command/SetKey.hs index 090edee0ba..5d6a6ca26d 100644 --- a/Command/SetKey.hs +++ b/Command/SetKey.hs @@ -33,7 +33,7 @@ perform file key = do -- the file might be on a different filesystem, so moveFile is used -- rather than simply calling moveAnnex; disk space is also -- checked this way. - ok <- getViaTmp DefaultVerify key $ \dest -> unVerified $ + ok <- getViaTmp RetrievalAllKeysSecure DefaultVerify key $ \dest -> unVerified $ if dest /= file then liftIO $ catchBoolIO $ do moveFile file dest diff --git a/Command/TestRemote.hs b/Command/TestRemote.hs index 8eeb2b5a69..baffbe131c 100644 --- a/Command/TestRemote.hs +++ b/Command/TestRemote.hs @@ -179,7 +179,7 @@ test st r k = Just b -> case Backend.verifyKeyContent b of Nothing -> return True Just verifier -> verifier k (key2file k) - get = getViaTmp (RemoteVerify r) k $ \dest -> + get = getViaTmp (Remote.retrievalSecurityPolicy r) (RemoteVerify r) k $ \dest -> Remote.retrieveKeyFile r k (AssociatedFile Nothing) dest nullMeterUpdate store = Remote.storeKey r k (AssociatedFile Nothing) nullMeterUpdate @@ -220,7 +220,7 @@ testExportTree st (Just _) ea k1 k2 = retrieveexport k = withTmpFile "exported" $ \tmp h -> do liftIO $ hClose h ifM (Remote.retrieveExport ea k testexportlocation tmp nullMeterUpdate) - ( verifyKeyContent AlwaysVerify UnVerified k tmp + ( verifyKeyContent RetrievalAllKeysSecure AlwaysVerify UnVerified k tmp , return False ) checkpresentexport k = Remote.checkPresentExport ea k testexportlocation @@ -238,10 +238,10 @@ testUnavailable st r k = , check (`notElem` [Right True, Right False]) "checkPresent" $ Remote.checkPresent r k , check (== Right False) "retrieveKeyFile" $ - getViaTmp (RemoteVerify r) k $ \dest -> + getViaTmp (Remote.retrievalSecurityPolicy r) (RemoteVerify r) k $ \dest -> Remote.retrieveKeyFile r k (AssociatedFile Nothing) dest nullMeterUpdate , check (== Right False) "retrieveKeyFileCheap" $ - getViaTmp (RemoteVerify r) k $ \dest -> unVerified $ + getViaTmp (Remote.retrievalSecurityPolicy r) (RemoteVerify r) k $ \dest -> unVerified $ Remote.retrieveKeyFileCheap r k (AssociatedFile Nothing) dest ] where diff --git a/Command/TransferKey.hs b/Command/TransferKey.hs index 1aa0a72771..b1f9515a8d 100644 --- a/Command/TransferKey.hs +++ b/Command/TransferKey.hs @@ -60,7 +60,7 @@ toPerform key file remote = go Upload file $ fromPerform :: Key -> AssociatedFile -> Remote -> CommandPerform fromPerform key file remote = go Upload file $ download (uuid remote) key file stdRetry $ \p -> - getViaTmp (RemoteVerify remote) key $ + getViaTmp (retrievalSecurityPolicy remote) (RemoteVerify remote) key $ \t -> Remote.retrieveKeyFile remote key file t p go :: Direction -> AssociatedFile -> (NotifyWitness -> Annex Bool) -> CommandPerform diff --git a/Command/TransferKeys.hs b/Command/TransferKeys.hs index 94582b2e07..b986a32714 100644 --- a/Command/TransferKeys.hs +++ b/Command/TransferKeys.hs @@ -42,7 +42,7 @@ start = do return ok | otherwise = notifyTransfer direction file $ download (Remote.uuid remote) key file stdRetry $ \p -> - getViaTmp (RemoteVerify remote) key $ \t -> do + getViaTmp (Remote.retrievalSecurityPolicy remote) (RemoteVerify remote) key $ \t -> do r <- Remote.retrieveKeyFile remote key file t p -- Make sure we get the current -- associated files data for the key, diff --git a/NEWS b/NEWS index 2dcc72e52b..445239fd23 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,11 @@ git-annex (6.20180622) upstream; urgency=high + A security fix has changed git-annex to refuse to download content from + some special remotes when the content cannot be verified with a hash check. + In particular URL and WORM keys stored on such remotes won't be downloaded. + See the documentation of the annex.security.allow-unverified-downloads + configuration for how to deal with this if it affects your files. + A security fix has changed git-annex to only support http, https, and ftp URL schemes by default. You can enable other URL schemes, at your own risk, using annex.security.allowed-url-schemes. diff --git a/P2P/Annex.hs b/P2P/Annex.hs index 05fa9e9ac0..008de23a50 100644 --- a/P2P/Annex.hs +++ b/P2P/Annex.hs @@ -22,6 +22,7 @@ import P2P.Protocol import P2P.IO import Logs.Location import Types.NumCopies +import Types.Remote (RetrievalSecurityPolicy(..)) import Utility.Metered import Control.Monad.Free @@ -63,9 +64,12 @@ runLocal runst runner a = case a of Right Nothing -> runner (next False) Left e -> return (Left (show e)) StoreContent k af o l getb validitycheck next -> do + -- This is the same as the retrievalSecurityPolicy of + -- Remote.P2P and Remote.Git. + let rsp = RetrievalAllKeysSecure ok <- flip catchNonAsync (const $ return False) $ transfer download k af $ \p -> - getViaTmp DefaultVerify k $ \tmp -> do + getViaTmp rsp DefaultVerify k $ \tmp -> do storefile tmp o l getb validitycheck p runner (next ok) StoreContentTo dest o l getb validitycheck next -> do diff --git a/Remote.hs b/Remote.hs index 4df907629b..ff891962a1 100644 --- a/Remote.hs +++ b/Remote.hs @@ -12,6 +12,7 @@ module Remote ( storeKey, retrieveKeyFile, retrieveKeyFileCheap, + retrievalSecurityPolicy, removeKey, hasKey, hasKeyCheap, diff --git a/Remote/Git.hs b/Remote/Git.hs index 3f85365acf..8a2ee9acaa 100644 --- a/Remote/Git.hs +++ b/Remote/Git.hs @@ -626,10 +626,11 @@ copyToRemote' repo r (State connpool duc _) key file meterupdate ensureInitialized copier <- mkCopier hardlink params let verify = Annex.Content.RemoteVerify r + let rsp = RetrievalAllKeysSecure runTransfer (Transfer Download u key) file stdRetry $ \p -> let p' = combineMeterUpdate meterupdate p in Annex.Content.saveState True `after` - Annex.Content.getViaTmp verify key + Annex.Content.getViaTmp rsp verify key (\dest -> copier object dest p' (liftIO checksuccessio)) ) copyremotefallback p = Annex.Content.sendAnnex key noop $ \object -> do diff --git a/Types/GitConfig.hs b/Types/GitConfig.hs index 7d9ccbff1f..26ad354c8d 100644 --- a/Types/GitConfig.hs +++ b/Types/GitConfig.hs @@ -96,6 +96,7 @@ data GitConfig = GitConfig , annexRetryDelay :: Maybe Seconds , annexAllowedUrlSchemes :: S.Set Scheme , annexAllowedHttpAddresses :: String + , annexAllowUnverifiedDownloads :: Bool , coreSymlinks :: Bool , coreSharedRepository :: SharedRepository , receiveDenyCurrentBranch :: DenyCurrentBranch @@ -166,6 +167,8 @@ extractGitConfig r = GitConfig getmaybe (annex "security.allowed-url-schemes") , annexAllowedHttpAddresses = fromMaybe "" $ getmaybe (annex "security.allowed-http-addresses") + , annexAllowUnverifiedDownloads = (== Just "ACKTHPPT") $ + getmaybe (annex "security.allow-unverified-downloads") , coreSymlinks = getbool "core.symlinks" True , coreSharedRepository = getSharedRepository r , receiveDenyCurrentBranch = getDenyCurrentBranch r diff --git a/doc/git-annex.mdwn b/doc/git-annex.mdwn index 5c84249763..163a628c15 100644 --- a/doc/git-annex.mdwn +++ b/doc/git-annex.mdwn @@ -1228,7 +1228,7 @@ Here are all the supported configuration settings. Note that even when this is set to `false`, git-annex does verification in some edge cases, where it's likely the case than an - object was downloaded incorrectly. + object was downloaded incorrectly, or when needed for security. * `remote..annex-export-tracking` @@ -1425,6 +1425,43 @@ Here are all the supported configuration settings. these IP address restrictions to be enforced, curl and youtube-dl will never be used unless annex.security.allowed-http-addresses=all. +* `annex.security.allow-unverified-downloads`, + + For security reasons, git-annex refuses to download content from + most special remotes when it cannot check a hash to verify + that the correct content was downloaded. This particularly impacts + downloading the content of URL or WORM keys, which lack hashes. + + The best way to avoid problems due to this is to migrate files + away from such keys, before their content reaches a special remote. + See [[git-annex-migrate]](1). + + When the content is only available from a special remote, you can + use this configuration to force git-annex to download it. + But you do so at your own risk, and it's very important you read and + understand the information below first! + + Downloading unverified content from encrypted special remotes is + prevented, because the special remote could send some other encrypted + content than what you expect, causing git-annex to decrypt data that you + never checked into git-annex, and risking exposing the decrypted + data to any non-encrypted remotes you send content to. + + Downloading unverified content from (non-encrypted) + external special remotes is prevented, because they could follow + http redirects to web servers on localhost or on a private network, + or in some cases to a file:/// url. + + If you decide to bypass this security check, the best thing to do is + to only set it temporarily while running the command that gets the file. + The value to set the config to is "ACKTHPPT". + For example: + + git -c annex.security.allow-unverified-downloads=ACKTHPPT annex get myfile + + It would be a good idea to check that it downloaded the file you expected, + too. + * `annex.secure-erase-command` This can be set to a command that should be run whenever git-annex From 838b65bd6b547586af6e7a8d772ae1f0de5dc3cf Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 13:45:07 -0400 Subject: [PATCH 28/41] update status --- ...hole_private_data_exposure_via_addurl.mdwn | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn index ae4f6f9857..12aeb9c8a0 100644 --- a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn +++ b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn @@ -119,10 +119,6 @@ CLAIMURL and CHECKURL, and then themselves download the content of urls in response to a TRANSFER RETRIEVE will have the same problems as git-annex's url downloading. -> Checked all known external special remotes -> to see if they used GETURLS. ipfs did, but not in an exploitable way. -> datalad does; looped in its developers to asess. Rest appear not to. - An external special remote might also make a simple http request to a key/value API to download a key, and follow a redirect to file:/// or http://localhost/. @@ -153,10 +149,19 @@ special remotes, to block the redirection attack. There could be a config setting to say that the git-annex repository is not being shared with untrusted third parties, and relax that check. -> TODO +> done -Tocho Tochev has updated git-annex-remote-pcloud to not follow http -redirects. +TODO Tighten down the gpg decryption to only allow decrypting with +the provided symmetric key, as a further protection against CVE-2018-10859. +If this can be done, then only remotes with encryption=pubkey will +really need to reject WORM and URL keys, since encryption=shared +and encryption=hybrid use a symetric key that's only used to encrypt data +for that remote. Although opening those back up to WORM and URL would +allow the remote sending other content stored on it, to get the wrong +content decrypted. This seems unlikely to be a useful exploit in most +cases, but perhaps not all cases, so probably best to not relax the +rejection aven when doing this. It's still worth doing as a belt and braces +fix. ---- From f1b29dbeb4277bf8a7febc795fe52e7b109c6d59 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 14:14:56 -0400 Subject: [PATCH 29/41] don't assume boto will remain secure On second thought, best to default to being secure even if boto changes http libraries to one that happens to follow redirects. --- Remote/Glacier.hs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Remote/Glacier.hs b/Remote/Glacier.hs index e6cd68fdf3..ad5f2e24a7 100644 --- a/Remote/Glacier.hs +++ b/Remote/Glacier.hs @@ -56,8 +56,10 @@ gen r u c gc = new <$> remoteCost gc veryExpensiveRemoteCost , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap this -- glacier-cli does not follow redirects and does - -- not support file://, so this is secure. - , retrievalSecurityPolicy = RetrievalAllKeysSecure + -- not support file://, as far as we know, but + -- there's no guarantee that will continue to be + -- the case, so require verifiable keys. + , retrievalSecurityPolicy = RetrievalVerifiableKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy From 66b14b5d66fe46b497d3bc775f7d2522c0de8fd6 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 14:50:20 -0400 Subject: [PATCH 30/41] devblog --- doc/devblog/day_499__security_hole.mdwn | 6 +++--- doc/devblog/day_504__security_hole_part_6.mdwn | 10 ++++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 doc/devblog/day_504__security_hole_part_6.mdwn diff --git a/doc/devblog/day_499__security_hole.mdwn b/doc/devblog/day_499__security_hole.mdwn index 9f0d532d6d..635dc3f073 100644 --- a/doc/devblog/day_499__security_hole.mdwn +++ b/doc/devblog/day_499__security_hole.mdwn @@ -4,9 +4,9 @@ now when the security hole is disclosed. Security is not compositional. You can have one good feature, and add another good feature, and the result is not two good features, but a new security hole. In this case -[[bugs/security_hole_private_data_exposure_via_addurl]]. And it can be hard -to spot this kind of security hole, but then once it's known it -seems blindly obvious. +[[bugs/security_hole_private_data_exposure_via_addurl]] (CVE-2018-10857). +And it can be hard to spot this kind of security hole, but then once it's +known it seems blindly obvious. It came to me last night and by this morning I had decided the potential impact was large enough to do a coordinated disclosure. Spent the first diff --git a/doc/devblog/day_504__security_hole_part_6.mdwn b/doc/devblog/day_504__security_hole_part_6.mdwn new file mode 100644 index 0000000000..7fb9cf1dc1 --- /dev/null +++ b/doc/devblog/day_504__security_hole_part_6.mdwn @@ -0,0 +1,10 @@ +Was getting dangerously close to burnt out, or exhaustion leading to +mistakes, so yesterday I took the day off, aside from spending the morning +babysitting the android build every half hour. (It did finally succeed.) + +Today, got back into it, and implemented a fix for CVE-2018-10859 and also +the one case of CVE-2018-10857 that had not been dealt with before. +This fix was really a lot easier than the previous fixes for +CVE-2018-10857. +Unfortunately this did mean not letting URL and WORM keys be downloaded +from many special remotes by default, which is going to be painful for some. From a5460132a675f693253cfc66b6c2f061943cb4a8 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 14:56:04 -0400 Subject: [PATCH 31/41] update version --- CHANGELOG | 2 +- NEWS | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 6f82a31b76..30680cfa11 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,4 +1,4 @@ -git-annex (6.20180622) upstream; urgency=high +git-annex (6.20180626) upstream; urgency=high Security fix release for CVE-2018-10857 and CVE-2018-10859 diff --git a/NEWS b/NEWS index 445239fd23..f757a23feb 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,4 @@ -git-annex (6.20180622) upstream; urgency=high +git-annex (6.20180626) upstream; urgency=high A security fix has changed git-annex to refuse to download content from some special remotes when the content cannot be verified with a hash check. From 4a89728d64c7b9425e6fe2d842e82205a50fa108 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 15:49:11 -0400 Subject: [PATCH 32/41] close --- ...hole_private_data_exposure_via_addurl.mdwn | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn index 12aeb9c8a0..1452415da3 100644 --- a/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn +++ b/doc/bugs/security_hole_private_data_exposure_via_addurl.mdwn @@ -149,9 +149,9 @@ special remotes, to block the redirection attack. There could be a config setting to say that the git-annex repository is not being shared with untrusted third parties, and relax that check. -> done +> done in [[!commit b657242f5d946efae4cc77e8aef95dd2a306cd6b]] -TODO Tighten down the gpg decryption to only allow decrypting with +Could also tighten down the gpg decryption to only allow decrypting with the provided symmetric key, as a further protection against CVE-2018-10859. If this can be done, then only remotes with encryption=pubkey will really need to reject WORM and URL keys, since encryption=shared @@ -163,6 +163,11 @@ cases, but perhaps not all cases, so probably best to not relax the rejection aven when doing this. It's still worth doing as a belt and braces fix. +> AFAICS, gpg does not have a way to specify to decrypt with only a +> symmetric encryption key. It could be done by running gpg in an +> environment with an empty keyring, but gpg agent makes that difficult and +> it would be added complexity. Decided not to do it. + ---- Built-in special remotes that use protocols on top of http, eg S3 and WebDAV, @@ -203,12 +208,7 @@ youtube-dl > > > done in [[!commit e62c4543c31a61186ebf2e4e0412df59fc8630c8]] -glacier -> This special remote uses glacier-cli, which will need to be audited. -> Emailed Robie Basak about it, and he looked into the http libraries -> used by glacier-cli and boto. It appears that they do not support -> file:///. It also appears that the libraries do not handle redirects -> themselves, and that boto does not handle http redirects. glacier-cli -> uses https. Combining all this, it seems that glacier-cli is not -> vulnerable to this class of attacks. +---- + +Both security holes are now fixed. [[done]] --[[Joey]] From 05ecee0db43919b3cfd8d2e2772022e16a804a04 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 16:38:47 -0400 Subject: [PATCH 33/41] set ddar to RetrievalAllKeysSecure Based on information from Robie Basak. --- Remote/Ddar.hs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Remote/Ddar.hs b/Remote/Ddar.hs index 1ca1fda159..4a6af3e451 100644 --- a/Remote/Ddar.hs +++ b/Remote/Ddar.hs @@ -58,8 +58,9 @@ gen r u c gc = do , storeKey = storeKeyDummy , retrieveKeyFile = retreiveKeyFileDummy , retrieveKeyFileCheap = retrieveCheap - -- Unsure about this, safe default until Robie answers. - , retrievalSecurityPolicy = RetrievalVerifiableKeysSecure + -- ddar communicates over ssh, not subject to http redirect + -- type attacks + , retrievalSecurityPolicy = RetrievalAllKeysSecure , removeKey = removeKeyDummy , lockContent = Nothing , checkPresent = checkPresentDummy From 787e46a44b0e06af8f4971eaa80bb1b4e7b79464 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 16:40:31 -0400 Subject: [PATCH 34/41] note that glacier was also limited --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 30680cfa11..2f80b391b8 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,7 +4,7 @@ git-annex (6.20180626) upstream; urgency=high * Refuse to download content, that cannot be verified with a hash, from encrypted special remotes (for CVE-2018-10859), - and from all external special remotes (for CVE-2018-10857). + and from all external special remotes and glacier (for CVE-2018-10857). In particular, URL and WORM keys stored on such remotes won't be downloaded. If this affects your files, you can run `git-annex migrate` on the affected files, to convert them From fff1825f13e64f21f9d782977480802dea701f7a Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 16:50:41 -0400 Subject: [PATCH 35/41] adjust version --- doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn b/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn index a5bf747529..9f2d6d95b6 100644 --- a/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn +++ b/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn @@ -9,7 +9,7 @@ key. This attack could be used to expose encrypted data that was never stored in git-annex. Daniel Dent discovered this attack in collaboration with Joey Hess. -These security holes were fixed in git-annex 6.20180622. +These security holes were fixed in git-annex 6.20180626. Also, some external special remotes (plugins) were improved, as a second line of defense against CVE-2018-10857: From 9faef71650951fe96a7a03fb14045ad8e94f446c Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 18:16:44 -0400 Subject: [PATCH 36/41] add upgrade note --- doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn b/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn index 9f2d6d95b6..e789a73d49 100644 --- a/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn +++ b/doc/security/CVE-2018-10857_and_CVE-2018-10859.mdwn @@ -9,7 +9,8 @@ key. This attack could be used to expose encrypted data that was never stored in git-annex. Daniel Dent discovered this attack in collaboration with Joey Hess. -These security holes were fixed in git-annex 6.20180626. +These security holes were fixed in git-annex 6.20180626. After upgrading +git-annex, you should restart any git-annex assistant processes. Also, some external special remotes (plugins) were improved, as a second line of defense against CVE-2018-10857: From eb8a8976a996105abab9cb81b46b8666c6c919c3 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 21 Jun 2018 20:54:02 -0400 Subject: [PATCH 37/41] comment --- Annex/Content.hs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Annex/Content.hs b/Annex/Content.hs index 8902114dad..cc0a0b3a6e 100644 --- a/Annex/Content.hs +++ b/Annex/Content.hs @@ -325,6 +325,13 @@ getViaTmpFromDisk rsp v key action = checkallowed $ do ) , do warning "verification of content failed" + -- The bad content is not retained, because + -- a retry should not try to resume from it + -- since it's apparently corrupted. + -- Also, the bad content could be any data, + -- including perhaps the content of another + -- file than the one that was requested, + -- and so it's best not to keep it on disk. pruneTmpWorkDirBefore tmpfile (liftIO . nukeFile) return False ) From 3976b89116daa1f97b1367bbcec3c7f1a0205d27 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 22 Jun 2018 10:22:04 -0400 Subject: [PATCH 38/41] fix license date I wrote this this year --- Utility/IPAddress.hs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Utility/IPAddress.hs b/Utility/IPAddress.hs index 3a61dc34a7..c180a5c459 100644 --- a/Utility/IPAddress.hs +++ b/Utility/IPAddress.hs @@ -1,6 +1,6 @@ {- IP addresses - - - Copyright 2012 Joey Hess + - Copyright 2018 Joey Hess - - License: BSD-2-clause -} From dab55715da142bf1877404bb7966a0d180a6c467 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 22 Jun 2018 10:27:22 -0400 Subject: [PATCH 39/41] add link to advistory --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 2f80b391b8..c7a8660d6f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ git-annex (6.20180626) upstream; urgency=high Security fix release for CVE-2018-10857 and CVE-2018-10859 + https://git-annex.branchable.com/security/CVE-2018-10857_and_CVE-2018-10859/ * Refuse to download content, that cannot be verified with a hash, from encrypted special remotes (for CVE-2018-10859), From 47cd6923b4b10f24d66dc2d405a72fa932efeec2 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 22 Jun 2018 10:30:10 -0400 Subject: [PATCH 40/41] mention new limitation --- doc/backends.mdwn | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/backends.mdwn b/doc/backends.mdwn index 3944af678b..435e2065da 100644 --- a/doc/backends.mdwn +++ b/doc/backends.mdwn @@ -53,6 +53,9 @@ contents that were checked into a repository earlier, you should avoid using the non-cryptographically-secure backends, and will need to use signed git commits. See [[tips/using_signed_git_commits]] for details. +Retrieval of WORM and URL from many [[special_remotes]] is prohibited +for [[security_reasons|security/CVE-2018-10857_and_CVE-2018-10859]]. + Note that the various 512 and 384 length hashes result in long paths, which are known to not work on Windows. If interoperability on Windows is a concern, avoid those. From 57dc30a029003c46724ab65f776031e958a47c42 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Fri, 22 Jun 2018 10:36:44 -0400 Subject: [PATCH 41/41] finalize release --- CHANGELOG | 2 +- git-annex.cabal | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index c7a8660d6f..e269691819 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -51,7 +51,7 @@ git-annex (6.20180626) upstream; urgency=high git-annex fsck --from remote has noticed it's gone, re-running git-annex export or git-annex sync --content will re-upload it. - -- Joey Hess Tue, 19 Jun 2018 11:40:21 -0400 + -- Joey Hess Fri, 22 Jun 2018 10:36:22 -0400 git-annex (6.20180529) upstream; urgency=medium diff --git a/git-annex.cabal b/git-annex.cabal index fc64a16651..a497eeae76 100644 --- a/git-annex.cabal +++ b/git-annex.cabal @@ -1,5 +1,5 @@ Name: git-annex -Version: 6.20180529 +Version: 6.20180626 Cabal-Version: >= 1.8 License: GPL-3 Maintainer: Joey Hess