I was implementing an option to turn off the malicious file path
check, for (if nothing else) backward compatibility with older
versions (or more exactly, how they are sometimes used). Then I
discovered that the check is not working properly, and that a
proper way of doing the check is not so easy to implement.
The main problem with the old way is the apply function in
PatchApply is not the only "entry point" to applying patches.
There doesn't seem to be a clean interface for patch applying
functions at all. A better way would of course be to put the
check in the writing functions, but I'm not so familiar with
that code. I couldn't spot a structure that showed which
functions to do the check in. But I remember something from when
this problem was discussed the first time about doing it in the
Slurpy. Any ideas? Another possibility is to do it in PatchRead,
but the opts argument is not (yet) propagated to those
functions, and the discussed state monad would be a much nicer
way to do that.
So I invented another solution, which turned out to be maybe not
so great after all. I wrote functions for testing a bunch of
patches, and intended to call them from all appropriate darcs
commands on patches before applying them. The last part was not
so easy. The problem is that really _all_ read patches need to
be checked, including pending and unrevert, and much of that
reading and applying goes on in deep function calls.
But anyway, this submission is a sort of minimal but much more
appropriate malicious path check. It unfortunately only checks
remote patches fetched with Apply or Pull, but it does check
them reliably. I would like to also check Get, or at least
Check. I tried with Check, but it made darcs silent for a too
long time, so some kind of progression feedback is needed. It
also needs a better way to inform the user what went wrong,
instead of the cryptic fail message "Malicious path".
But most of all, this is on/off-toggable, so that those who need
to turn the check off can upgrade, and therefore I'd want it to
go into the upcoming stable release. And I have time for darcs
hacking now, so please demand improvements. :-)
Thu Nov 9 15:41:44 CET 2006 Tommy Pettersson <[EMAIL PROTECTED]>
* add function for finding all file names in a patch
Fri Nov 10 14:23:38 CET 2006 Tommy Pettersson <[EMAIL PROTECTED]>
* add new malicious file path check system
Adds a new module DarcsCommandsAux for auxiliary functionality common to
more than one darcs command.
Fri Nov 10 21:55:11 CET 2006 Tommy Pettersson <[EMAIL PROTECTED]>
* fix latex markup error
Fri Nov 10 22:17:02 CET 2006 Tommy Pettersson <[EMAIL PROTECTED]>
* use new malicious file path check in pull and apply (issue177)
Fri Nov 10 22:17:57 CET 2006 Tommy Pettersson <[EMAIL PROTECTED]>
* remove old malicious_filename check (issue177)
New patches:
[add function for finding all file names in a patch
Tommy Pettersson <[EMAIL PROTECTED]>**20061109144144]
<
> {
hunk ./Patch.lhs 66
list_conflicted_files, list_touched_files,
resolve_conflicts,
merger_equivalent, modernize_patch,
+ filenames,
-- for Population
DirMark(..), patchChanges, applyToPop,
-- for PatchTest
hunk ./Patch.lhs 80
null_patch, is_null_patch, infopatch, invert,
flatten, flatten_to_primitives, is_adddir,
is_addfile, is_hunk, is_binary, is_merger, is_setpref,
- is_similar, patch2patchinfo, patchname )
+ is_similar, patch2patchinfo, patchname, filenames )
import PatchRead ( readPatch, readPatchLazily, gzReadPatchFileLazily )
import PatchShow ( writePatch, gzWritePatch, showPatch )
import PatchViewing ( patch_summary, xml_summary, patch_description,
hunk ./PatchCore.lhs 28
hunk, move, namepatch, rmdir, rmfile, tokreplace,
join_patches, unjoin_patches, getdeps,
is_addfile, is_hunk, is_binary, is_merger, is_setpref,
- is_similar, is_adddir, patch2patchinfo, patchname )
+ is_similar, is_adddir, patch2patchinfo, patchname,
+ filenames,
+ )
where
import Prelude hiding ( pi )
hunk ./PatchCore.lhs 254
invert (ComP ps) = ComP (map invert (reverse ps))
invert (Split ps) = Split (map invert (reverse ps))
\end{code}
+
+\begin{code}
+-- Recurse on everything, these are potentially spoofed patches
+filenames :: Patch -> [FileName]
+filenames (NamedP _ _ p) = filenames p
+filenames (Move f1 f2) = [f1, f2]
+filenames (DP f _) = [f]
+filenames (FP f _) = [f]
+filenames (Split ps) = concatMap filenames ps
+filenames (ComP ps) = concatMap filenames ps
+filenames (Merger _ _ p ps p1 p2) = concatMap filenames (p:p1:p2:ps)
+filenames (Conflictor _ ps1 ps2) = concatMap filenames (ps1++ps2)
+filenames (ChangePref _ _ _) = []
+\end{code}
}
[add new malicious file path check system
Tommy Pettersson <[EMAIL PROTECTED]>**20061110132338
Adds a new module DarcsCommandsAux for auxiliary functionality common to
more than one darcs command.
]
<
> {
hunk ./DarcsArguments.lhs 23
DarcsOption( .. ), option_from_darcsoption,
help, verbose, list_options, list_files,
help_on_match,
- quiet, any_verbosity, disable,
+ quiet, any_verbosity, disable, restrict_paths,
notest, test, working_repo_dir,
leave_test_dir,
possibly_remote_repo_dir, get_repodir,
hunk ./DarcsArguments.lhs 193
working_repo_dir :: DarcsOption
possibly_remote_repo_dir :: DarcsOption
disable :: DarcsOption
+restrict_paths :: DarcsOption
gui_interactive, pipe_interactive, gui_pipe_interactive, all_gui_interactive,
all_gui_pipe_interactive, all_interactive, all_patches, interactive, pipe,
hunk ./DarcsArguments.lhs 303
get_repodir (RepoDir r:_) = r
get_repodir (_:fs) = get_repodir fs
\end{code}
-
+
\input{Match.lhs}
\input{PatchMatch.lhs}
hunk ./DarcsArguments.lhs 1110
DarcsArgOption [] ["umask"] UMask "UMASK"
"specify umask to use when writing."
\end{code}
+
+\begin{options}
+--dont-restrict-paths, --restrict-paths
+\end{options}
+By default darcs is only allowed to manage and modify files and directories
+contained inside the current repository and not being part of any darcs
+repository's meta data (including the current one). This is mainly for
+security, to protect you from spoofed patches modifying arbitrary files
+with sensitive data---say, in your home directory---or tempering with any
+repository's meta data to switch off this safety guard.
+
+But sometimes you may want to manage a group of ``sub'' repositories'
+preference files with a global repository, or use darcs in some other
+advanced way. The best way is probably to put \verb!ALL
+dont-restrict-paths! in \verb!_darcs/prefs/defaults!. This turns off all
+sanity checking for file paths in patches.
+
+Path checking can be temporarily turned on with \verb!--restrict-paths! on
+the command line, when pulling or applying unknown patches.
+
+\begin{code}
+restrict_paths =
+ DarcsMultipleChoiceOption
+ [DarcsNoArgOption [] ["restrict-paths"] RestrictPaths
+ "don't allow darcs to touch external files or repo metadata",
+ DarcsNoArgOption [] ["dont-restrict-paths"] DontRestrictPaths
+ "allow darcs to modify any file or directory (unsafe)"]
+\end{code}
addfile ./DarcsCommandsAux.lhs
hunk ./DarcsCommandsAux.lhs 1
+% Copyright (C) 2006 Tommy Pettersson <[EMAIL PROTECTED]>
+%
+% This program is free software; you can redistribute it and/or modify
+% it under the terms of the GNU General Public License as published by
+% the Free Software Foundation; either version 2, or (at your option)
+% any later version.
+%
+% This program is distributed in the hope that it will be useful,
+% but WITHOUT ANY WARRANTY; without even the implied warranty of
+% MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+% GNU General Public License for more details.
+%
+% You should have received a copy of the GNU General Public License
+% along with this program; if not, write to the Free Software Foundation,
+% Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
+\begin{code}
+module DarcsCommandsAux ( check_paths, malicious_patches, has_malicious_path,
+ ) where
+import DarcsFlags ( DarcsFlag( RestrictPaths, DontRestrictPaths ) )
+import FileName ( fn2fp, breakup, is_explicitly_relative )
+import Patch ( Patch, filenames )
+import Data.List ( intersect )
+\end{code}
+
+\begin{code}
+{-
+ FILE PATHS
+
+ Darcs will operate on files and directories with the invoking user's
+ privileges. The paths for these files and directories are stored in
+ patches, which darcs receives in various ways. Even though darcs will not
+ create patches with "unexpected" file paths, there are no such guarantees
+ for received patches. A spoofed patch could inflict changes on any file
+ or directory which the invoking user is privileged to modify.
+
+ There is no one single "apply" function that can check paths, so each
+ command is responsible for not applying patches without first checking
+ them with one of these function when appropriate.
+-}
+
+check_paths :: [DarcsFlag] -> [Patch] -> IO ()
+{-
+ A convenience function to call from all darcs command functions before
+ applying any patches. It checks for malicious paths in patches, and
+ prints an error message and fails if it finds one.
+-}
+check_paths opts patches
+ = if check_is_on && any has_malicious_path patches
+ then fail "Malicious path"
+ -- TODO: print patch(es) and path(s)
+ -- NOTE: should use safe Doc printer, this can be evil chars
+ else return ()
+ where
+ check_is_on = DontRestrictPaths `notElem` opts ||
+ RestrictPaths `elem` opts
+
+malicious_patches :: [Patch] -> [Patch]
+-- Filter out pathces that contains some malicious file path
+malicious_patches to_check = filter has_malicious_path to_check
+
+has_malicious_path :: Patch -> Bool
+has_malicious_path patch =
+ let paths = map fn2fp (filenames patch) in
+ any is_malicious_path paths
+
+{-
+ What is a malicious path?
+
+ A spoofed path is a malicious path.
+
+ 1) Darcs only creates explicitly relative paths (beginning with "./"), so
+ any not explicitly relative path is surely spoofed.
+
+ 2) Darcs normalizes paths so they never contain "/../", so paths with
+ "/../" are surely spoofed.
+
+ A path to a darcs repository's meta data can modify "trusted" patches or
+ change safety defaults in that repository, so we check for paths
+ containing "/_darcs/" which is the entry to darcs meta data.
+
+ To do?
+
+ * How about get repositories?
+
+ * Would it be worth adding a --semi-safe-paths option for allowing
+ changes to certain preference files (_darcs/prefs/) in sub
+ repositories'?
+-}
+
+is_malicious_path :: String -> Bool
+is_malicious_path fp =
+ not $ is_explicitly_relative fp &&
+ null (intersect (breakup fp) [ "..", "_darcs" ])
+\end{code}
hunk ./DarcsFlags.lhs 36
| LogFile String | RmLogFile
| DistName String | All
| Recursive | NoRecursive | Reorder
+ | RestrictPaths | DontRestrictPaths
| AskDeps | NoAskDeps | IgnoreTimes | LookForAdds | NoLookForAdds
| AnyOrder | CreatorHash String
| Intersection | Union
hunk ./GNUmakefile 23
COMMON_FILES := Autoconf.lhs \
CheckFileSystem.lhs ColourPrinter.lhs Compat.hs Curl.hs DarcsIO.lhs \
Pristine.lhs DarcsArguments.lhs DarcsFlags.lhs DarcsUtils.lhs \
- CommandLine.lhs \
+ CommandLine.lhs DarcsCommandsAux.lhs \
DateMatcher.lhs Depends.lhs Diff.lhs Exec.lhs External.hs \
FastPackedString.hs FileName.lhs FilePathMonad.lhs \
FilePathUtils.hs IsoDate.lhs Lcs.lhs Lock.lhs Map.hs \
}
[fix latex markup error
Tommy Pettersson <[EMAIL PROTECTED]>**20061110205511]
<
> {
hunk ./DarcsArguments.lhs 1123
But sometimes you may want to manage a group of ``sub'' repositories'
preference files with a global repository, or use darcs in some other
-advanced way. The best way is probably to put \verb!ALL
-dont-restrict-paths! in \verb!_darcs/prefs/defaults!. This turns off all
-sanity checking for file paths in patches.
+advanced way. The best way is probably to put
+\verb!ALL dont-restrict-paths! in \verb!_darcs/prefs/defaults!. This turns
+off all sanity checking for file paths in patches.
Path checking can be temporarily turned on with \verb!--restrict-paths! on
the command line, when pulling or applying unknown patches.
}
[use new malicious file path check in pull and apply (issue177)
Tommy Pettersson <[EMAIL PROTECTED]>**20061110211702]
<
> {
hunk ./Apply.lhs 29
import SignalHandler ( withSignalsBlocked )
import DarcsCommands ( DarcsCommand(..) )
+import DarcsCommandsAux ( check_paths )
import DarcsArguments ( DarcsFlag( Reply, Test, NoTest, AnyOrder,
Gui, Interactive, All,
MarkConflicts,
hunk ./Apply.lhs 151
when (null to_be_applied) $
do putStrLn "You don't want to apply any patches, so I'm exiting!"
exitWith ExitSuccess
+ check_paths opts to_be_applied
redirect_output opts from_whom $ do
when am_verbose $ putStrLn "We have the following extra patches:"
when am_verbose $ putDocLn $ vcat $ map (human_friendly.fst) $ head us'
hunk ./Check.lhs 27
import List ( sort )
import DarcsCommands ( DarcsCommand(..), nodefaults )
+import DarcsCommandsAux ( check_paths )
import DarcsArguments ( DarcsFlag( Quiet, Verbose, NoTest, LeaveTestDir ),
partial_check, any_verbosity, notest,
leave_test_dir, working_repo_dir,
hunk ./Check.lhs 113
case patch2patchinfo chk of
Just chtg -> do
putVerbose $ text "I am checking from a checkpoint."
- apply_patches_with_feedback [] False feedback putInfo
- $ (chtg, Just chk)
- : reverse (concat $ get_patches_beyond_tag chtg patches)
+ let ps = (chtg, Just chk) :
+ reverse (concat $ get_patches_beyond_tag chtg patches)
+ check_paths opts $ map (fromJust.snd) ps
+ apply_patches_with_feedback [] False feedback putInfo ps
Nothing -> impossible
hunk ./Check.lhs 118
- Nothing -> apply_patches_with_feedback [] False feedback putInfo $
- reverse $ concat patches
+ Nothing -> do check_paths opts $ map (fromJust.snd) (concat patches)
+ apply_patches_with_feedback [] False feedback putInfo $
+ reverse $ concat patches
is_same <-
withCurrentDirectory cwd $ (identifyPristine >>= checkPristine chd)
if is_same
}
[remove old malicious_filename check (issue177)
Tommy Pettersson <[EMAIL PROTECTED]>**20061110211757]
<
> {
hunk ./FileName.lhs 26
fn2ps, ps2fn,
break_on_dir, norm_path, own_name, super_name, patch_filename,
(///),
- is_malicious_filename,
+ breakup, is_explicitly_relative,
) where
import IO
hunk ./FileName.lhs 139
\end{code}
\begin{code}
-is_malicious_filename :: FileName -> Bool
-is_malicious_filename fn =
- not (is_explicitly_relative fp) ||
- breakup fp `contains_any` [ "..", "_darcs" ]
- where
- fp = fn2fp fn
- is_explicitly_relative ('.':'/':_) = True -- begins with "./"
- is_explicitly_relative _ = False
- contains_any hay needles = any (`elem` hay) needles
+is_explicitly_relative :: String -> Bool
+is_explicitly_relative ('.':'/':_) = True -- begins with "./"
+is_explicitly_relative _ = False
\end{code}
hunk ./PatchApply.lhs 33
break_after_nth_newline,
break_before_nth_newline, )
import FileName ( fn2ps, fn2fp, fp2fn,
- is_malicious_filename,
)
import PatchInfo ( PatchInfo )
import PopulationData ( Population(..), PopTree(..), Info(..), DirMark(..) )
hunk ./PatchApply.lhs 115
-- errors. When in doubt, set to False.
-> Patch -> m ()
--- catch malicious paths
-apply _ _ (FP f _) | is_malicious_filename f
- = fail ("malicious file patch name: " ++ (fn2fp f))
-apply _ _ (DP f _) | is_malicious_filename f
- = fail ("malicious dir patch name: " ++ (fn2fp f))
-apply _ _ (Move f _) | is_malicious_filename f
- = fail ("malicious move patch name: " ++ (fn2fp f))
-apply _ _ (Move _ f) | is_malicious_filename f
- = fail ("malicious move patch name: " ++ (fn2fp f))
-
apply opts isW (NamedP _ _ p) = apply opts isW p
apply opts isW p | is_merger p = apply opts isW (merger_equivalent p)
apply _ _ (Merger _ _ _ _ _ _) = impossible
}
Context:
[Look for Text.Regex in package regex-compat. Needed for GHC 6.6
Josef Svenningsson <[EMAIL PROTECTED]>**20061004123158]
[bumb version to 1.0.9rc2
Tommy Pettersson <[EMAIL PROTECTED]>**20061009204226]
[TAG 1.0.9rc1
Tommy Pettersson <[EMAIL PROTECTED]>**20061008175207]
Patch bundle hash:
61c3f1d89b120e72ad8a09daff0ae73e9beeb21b
_______________________________________________
darcs-devel mailing list
[email protected]
http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
