From 2b478ed505a7ce937e5904e3bf8d8bcd5dd09efd Mon Sep 17 00:00:00 2001 From: Damien De Paoli Date: Fri, 3 Feb 2023 16:57:39 +1100 Subject: [PATCH] made SafePath and ensured initital paths and move_paths have valid paths, and all filenames have to be found via os.walk so should be impossible to write to parts of the FS that are unsafe --- TODO | 8 +--- pa_job_manager.py | 98 ++++++++++++++++++++++++++++++++++------------- settings.py | 8 ++-- 3 files changed, 78 insertions(+), 36 deletions(-) diff --git a/TODO b/TODO index 6d38fc9..e876ab4 100644 --- a/TODO +++ b/TODO @@ -1,10 +1,4 @@ ### GENERAL - * think about security - in job_mgr anywhere I can os.replace/remove NEED to protect, etc - - just need to use this I think: - from werkzeug.utils import secure_filename - - secure_filename(xxxx) - * change the rotation code to use that jpeg util to reduce/remove compression loss? * read this: https://flask.palletsprojects.com/en/2.2.x/testing/#faking-resources-and-context @@ -34,6 +28,8 @@ files.py:@app.route("/fix_dups", methods=["POST"]) ??? + * allow user to choose default log level to show + * GUI overhaul? * on a phone, the files.html page header is a mess "Oldest.." line is too large to fit on 1 line (make it a hamburger?) - searched for text overlaps buttons above and below diff --git a/pa_job_manager.py b/pa_job_manager.py index 598fce1..0e3affd 100644 --- a/pa_job_manager.py +++ b/pa_job_manager.py @@ -577,14 +577,18 @@ def MessageToFE( job_id, message, level, persistent, cant_close ): def SettingsRBPath(): settings = session.query(Settings).first() if settings == None: - print("Cannot create file data with no settings / recycle bin path is missing") - return + print("ERROR: Cannot create file data with no settings / recycle bin path is missing") + return None # path setting is an absolute path, just use it, otherwise prepend base_path first - p=settings.recycle_bin_path - if p == '/': - return p + if settings.recycle_bin_path[0] == '/': + path=settings.recycle_bin_path else: - return settings.base_path+p + path=settings.base_path+settings.recycle_bin_path + if SafePath(path): + return path + else: + print ("ERROR: recycle bin path contains unsafe characters") + return None ############################################################################## @@ -593,7 +597,7 @@ def SettingsRBPath(): ############################################################################## def ProcessRecycleBinDir(job): path = SettingsRBPath() - if not os.path.exists( path ): + if path == None or not os.path.exists( path ): AddLogForJob( job, f"Not Importing {path} -- Path does not exist" ) return @@ -611,13 +615,17 @@ def ProcessRecycleBinDir(job): def SettingsSPath(): settings = session.query(Settings).first() if settings == None or settings.storage_path == "": - print("Cannot create file data with no settings / storage path is missing") - return - p=settings.storage_path - if p[0] == '/': - return p + print("ERROR: Cannot create file data with no settings / storage path is missing") + return None + if settings.storage_path[0] == '/': + path=settings.storage_path else: - return settings.base_path+p + path=settings.base_path+settings.storage_path + if SafePath(path): + return path + else: + print ("ERROR: storage path contains unsafe characters") + return None ############################################################################## # ProcessStorageDirs(): wrapper func to call passed in job for each @@ -636,15 +644,18 @@ def ProcessStorageDirs(parent_job): def SettingsIPath(): paths=[] settings = session.query(Settings).first() - if settings == None or settings.import_path == "": - print("Cannot create file data with no settings / import path is missing") - return - p=settings.import_path - if p[0] == '/': - return p + if not settings or settings.import_path == "": + print("ERROR: Cannot create file data with no settings / import path is missing") + return None + if settings.import_path[0] == '/': + path=settings.import_path else: - return settings.base_path+p - + path=settings.base_path+settings.import_path + if SafePath(path): + return path + else: + print ("ERROR: import path contains unsafe characters") + return None ############################################################################## # SettingsMPath(): return path to actual metadata path from settings @@ -652,13 +663,29 @@ def SettingsIPath(): def SettingsMPath(): settings = session.query(Settings).first() if not settings or settings.metadata_path == "": - print ("WARNING: no Settings for metadata path") + print ("ERROR: no Settings for metadata path") return None - p=settings.metadata_path - if p[0] == '/': - return p + if settings.metadata_path[0] == '/': + path=settings.metadata_path else: - return settings.base_path+p + path=settings.base_path+settings.metadata_path + if SafePath(path): + return path + else: + print ("ERROR: metadata path contains unsafe characters") + return None + + +############################################################################## +# SafePath(): checks to see if a path is safe (for now basic) checks: +# no .. +############################################################################## +def SafePath(path): + if '..' in path: + return False + else: + return True + ############################################################################## @@ -737,6 +764,7 @@ def CleanFileFromBin(job, e): # use ctime as that will be when the file was moved into the Bin path if (now - stat.st_ctime)/SECS_IN_A_DAY >= settings.bin_cleanup_file_age: try: + # SAFE: as fname is constructed from entry on DB, had to exist on FS to be in the DB (no user-input) os.remove( fname ) except Exception as ex: AddLogForJob(job, f"ERROR: Tried to delete old file: {ex}" ) @@ -778,11 +806,13 @@ def JobMetadata(job): if which == 'add_force_match_override' or which=='remove_force_match_override': person_id=[jex.value for jex in job.extra if jex.name == "person_id"][0] p=session.query(Person).get(person_id) + # SAFE: as dir is constructed from SafePath(Mpath) + data I control, no direct user-input os.makedirs( f"{SettingsMPath()}force_match_overrides", mode=0o777, exist_ok=True ) fname=f"{SettingsMPath()}force_match_overrides/{face_id}_{p.tag}" elif which == 'add_no_match_override' or which == 'remove_no_match_override': type_id=[jex.value for jex in job.extra if jex.name == "type_id"][0] t=session.query(FaceOverrideType).get(type_id) + # SAFE: as dir is constructed from SafePath(Mpath) + data I control, no direct user-input os.makedirs( f"{SettingsMPath()}no_match_overrides", mode=0o777, exist_ok=True ) fname=f"{SettingsMPath()}no_match_overrides/{face_id}_{t.name}" else: @@ -793,6 +823,7 @@ def JobMetadata(job): file_h.write(f.face) file_h.close() else: + # SAFE: as fname is constructed from SafePath(Mpath) + data I control, no direct user-input os.remove( fname ) except Exception as ex: AddLogForJob(job, f"ERROR: Error with metadata file '{fname}': {ex}" ) @@ -1003,6 +1034,7 @@ def DisconnectSingleNoMatchOverride( job, o ): new_fname=f'{mpath}0_{ot.name}_{md5face(f.face)}' try: if os.path.exists( fname ): + # SAFE: as SafePaths(mpath) combined with data I control in this func os.replace( fname, new_fname ) else: file_h=open( new_fname, 'wb') @@ -1038,6 +1070,7 @@ def DisconnectSingleForceMatchOverride( job, o ): new_fname=f'{path}0_{p.tag}_{md5face(f.face)}' try: if os.path.exists( fname ): + # SAFE: as SafePaths(mpath) combined with data I control in this func os.replace( fname, new_fname ) else: file_h=open( new_fname, 'wb') @@ -1103,6 +1136,7 @@ def CreateSymlink(job,ptype,path): if not os.path.exists(symlink): print( f"INFO: symlink does not exist, actually creating it -- s={symlink}" ) try: + # SAFE: SafePath() on init forces symlink to be safe os.makedirs( os.path.dirname(symlink), mode=0o777, exist_ok=True ) os.symlink(path, symlink) except Exception as e: @@ -1268,9 +1302,11 @@ def RestoreFile(job,restore_me): try: # rel_path for a file in the Bin, is like 'Import/images_to_process/1111', so just prepend static/ dst_dir='static/' + restore_me.in_dir.rel_path + '/' + # SAFE: as SafePaths(rbpath) combined with data I control in this func (explicit 'static/' + DB entry path) os.makedirs( dst_dir,mode=0o777, exist_ok=True ) src=restore_me.FullPathOnFS() dst=dst_dir + '/' + restore_me.name + # SAFE: as SafePaths(rbpath) combined with data I control in this func (explicit 'static/' + DB entry path) os.replace( src, dst ) except Exception as e: AddLogForJob( job, f"ERROR: Failed to restores (mv) file on filesystem - which={src} to {dst}, err: {e}") @@ -1321,6 +1357,7 @@ def MoveFileToRecycleBin(job,del_me): os.makedirs( dst_dir,mode=0o777, exist_ok=True ) src=del_me.FullPathOnFS() dst=dst_dir + '/' + del_me.name + # SAFE: as SafePaths(rbpath) combined with data I control in this func (explicit remove of 'static/' + DB entry path) os.replace( src, dst ) if DEBUG: print( f"MoveFileToRecycleBin({job.id},{del_me.name}): os.replace {src} with {dst} " ) @@ -1397,6 +1434,7 @@ def MoveEntriesToOtherFolder(job, move_me, dst_storage_path, dst_rel_path): ResetAnySubdirPaths( move_me, dst_storage_path, move_me.dir_details.rel_path ) # move the actual dir to its new location try: + # SAFE: paths are from DB (entries already existed on FS, no user-input) os.replace( orig_fs_pos, move_me.FullPathOnFS() ) except Exception as e: AddLogForJob( job, f"ERROR: Failed to move dir: {orig_fs_pos} into {move_me.FullPathOnFS()}, err: {e}") @@ -1416,6 +1454,7 @@ def MoveEntriesToOtherFolder(job, move_me, dst_storage_path, dst_rel_path): session.add(move_me) ResetAnySubdirPaths( move_me, dst_storage_path, dst_rel_path ) try: + # SAFE: paths are from DB (entries already existed on FS, no user-input) os.replace( orig_fs_pos, move_me.FullPathOnFS() ) except Exception as e: AddLogForJob( job, f"ERROR: Failed to rename dir: {orig_fs_pos} -> {move_me.FullPathOnFS()}, err: {e}") @@ -1441,6 +1480,7 @@ def MoveEntriesToOtherFolder(job, move_me, dst_storage_path, dst_rel_path): # move the actual file to its new location AddLogForJob( job, f"DEBUG: move of FILE - {orig_fs_pos} -> {move_me.FullPathOnFS()}" ) try: + # SAFE: paths are from DB (entries already existed on FS, no user-input) os.replace( orig_fs_pos, move_me.FullPathOnFS() ) except Exception as e: AddLogForJob( job, f"ERROR: Failed to move file: {orig_fs_pos} -> {move_me.FullPathOnFS()}, err: {e}") @@ -2018,6 +2058,7 @@ def GenVideoThumbnail( job, fname): .run(capture_stdout=True, capture_stderr=True) ) thumbnail, w, h = GenThumb( tmp_fname, False ) + # SAFE: as tmp_fname is constructed from data I control in this func and data from entry which had to exist on FS to be in the DB (no user-input) os.remove( tmp_fname ) except ffmpeg.Error as e: AddLogForJob( job, f"ERROR: Failed to Generate thumbnail for video file: {fname} - error={e}" ) @@ -2135,6 +2176,7 @@ def JobMoveFiles(job): JobProgressState( job, "In Progress" ) prefix=[jex.value for jex in job.extra if jex.name == "prefix"][0] suffix=[jex.value for jex in job.extra if jex.name == "suffix"][0] + # SAFE: this stops any user-input into the move call that could flow into future os.* calls # Sanity check, if prefix starts with /, reject it -> no /etc/shadow potentials # Sanity check, if .. in prefix or suffix, reject it -> no ../../etc/shadow potentials # Sanity check, if // in prefix or suffix, reject it -> not sure code wouldnt try to make empty dirs, and I dont want to chase /////// cases, any 2 in a row is enough to reject @@ -2264,6 +2306,7 @@ def ReloadMetadata(job): session.add( DisconnectedNoMatchOverride( face=face_data, type_id=otype.id ) ) if face_id: try: + # SAFE: as SafePaths(mpath) combined with data I control in this func os.replace( fname, f'{mpath}no_match_overrides/0_{otype.name}_{md5face(face_data)}' ) except Exception as ex: print( f"ERROR: renaming no-match metadata on filesystem failed: {ex}" ) @@ -2292,6 +2335,7 @@ def ReloadMetadata(job): # if face>0, then we need to move the FS copy to a disco if face_id: try: + # SAFE: as SafePaths(mpath) combined with data I control in this func os.replace( fname, f'{mpath}force_match_overrides/0_{p.tag}_{md5face(face_data)}' ) except Exception as ex: print( f"ERROR: renaming force-match metadata on filesystem failed: {ex}" ) @@ -2430,6 +2474,7 @@ def AddFaceToFile( locn_data, face_data, file_eid, model_id, settings ): # can only be 1 match with the * being a UUID fname=glob.glob( f'{path}0_{p.tag}_*' )[0] new_fname=f'{path}{face.id}_{p.tag}' + # SAFE: as SafePaths(mpath) combined with data I control in this func os.replace( fname, new_fname ) except Exception as ex: print( f"ERROR: AddFaceToFile-face connects to 'disconnected-force-match' metadata, but fixing the filesystem metadata failed: {ex}" ) @@ -2445,6 +2490,7 @@ def AddFaceToFile( locn_data, face_data, file_eid, model_id, settings ): # can only be 1 match with the * being a UUID fname=glob.glob( f'{path}0_{t.name}_*' )[0] new_fname=f'{path}{face.id}_{t.name}' + # SAFE: as SafePaths(mpath) combined with data I control in this func os.replace( fname, new_fname ) except Exception as ex: print( f"ERROR: AddFaceToFile-face connects to 'disconnected-no-match' metadata, but fixing the filesystem metadata failed: {ex}" ) diff --git a/settings.py b/settings.py index 66a6251..fa114e5 100644 --- a/settings.py +++ b/settings.py @@ -185,8 +185,8 @@ def SettingsMPath(): if not settings or settings.metadata_path == "": print ("WARNING: no Settings for metadata path") return - p=settings.metadata_path - if p[0] == '/': - return p + if settings.metadata_path[0] == '/': + path=settings.metadata_path else: - return settings.base_path+p + path=settings.base_path+settings.metadata_path + return path