From d1113d84d328a6f19a011cb60abab2e3261d1608 Mon Sep 17 00:00:00 2001 From: Allison Larson Date: Mon, 20 Oct 2025 16:18:12 -0700 Subject: [PATCH 1/2] Handle failed decompressed files When untarring a tarball, attempt to create files initially with the umask file permissions and delete the file if it fails to copy correctly. This prevents a paritally written file from remaining on disk with permissions that didn't include the umask. Adds a unit test and a invalid tarball (created by truncating a large file generated from /dev/urandom), which fails to decompress but errors partially written. --- decompress_tar_test.go | 27 +++++++++++++++++++++++++++ get_file_copy.go | 8 ++++++-- test-fixtures/decompress-tar/bad.tar | Bin 0 -> 9000 bytes 3 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 test-fixtures/decompress-tar/bad.tar diff --git a/decompress_tar_test.go b/decompress_tar_test.go index f7215f49c..ead311ef3 100644 --- a/decompress_tar_test.go +++ b/decompress_tar_test.go @@ -6,6 +6,7 @@ package getter import ( "archive/tar" "bytes" + "errors" "os" "path/filepath" "runtime" @@ -184,3 +185,29 @@ func TestDecompressTarPermissions(t *testing.T) { expected["directory/setuid"] = masked testDecompressorPermissions(t, d, input, expected, os.FileMode(060000000)) } + +func TestDecompressTarPermissionsFailed(t *testing.T) { + d := new(TarDecompressor) + input := "./test-fixtures/decompress-tar/bad.tar" + + td := t.TempDir() + + // Destination is always joining result so that we have a new path + dst := filepath.Join(td, "subdir", "result") + + err := d.Decompress(dst, input, true, os.FileMode(0)) + if err == nil { + t.Fatalf("expected error when decompressing bad tar file but got none") + } + + expectedDst := filepath.Join(dst, "directory/setuid2") + // Attempt to get file information + _, err = os.Stat(expectedDst) + + if !errors.Is(err, os.ErrNotExist) { + if err != nil { + t.Fatalf("unexpected error when checking for file '%s': %s", expectedDst, err) + } + t.Fatalf("expected file '%s' to not exist", expectedDst) + } +} diff --git a/get_file_copy.go b/get_file_copy.go index b64ae2ddf..1bcb4a4bd 100644 --- a/get_file_copy.go +++ b/get_file_copy.go @@ -35,7 +35,7 @@ func Copy(ctx context.Context, dst io.Writer, src io.Reader) (int64, error) { // copyReader copies from an io.Reader into a file, using umask to create the dst file func copyReader(dst string, src io.Reader, fmode, umask os.FileMode, fileSizeLimit int64) error { - dstF, err := os.OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, fmode) + dstF, err := os.OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, mode(fmode, umask)) if err != nil { return err } @@ -47,6 +47,8 @@ func copyReader(dst string, src io.Reader, fmode, umask os.FileMode, fileSizeLim _, err = io.Copy(dstF, src) if err != nil { + // Remove the file in case of partial write + _ = os.Remove(dst) return err } @@ -74,7 +76,7 @@ func copyFile(ctx context.Context, dst, src string, disableSymlinks bool, fmode, } defer func() { _ = srcF.Close() }() - dstF, err := os.OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, fmode) + dstF, err := os.OpenFile(dst, os.O_RDWR|os.O_CREATE|os.O_TRUNC, mode(fmode, umask)) if err != nil { return 0, err } @@ -82,6 +84,8 @@ func copyFile(ctx context.Context, dst, src string, disableSymlinks bool, fmode, count, err := Copy(ctx, dstF, srcF) if err != nil { + // Remove the file in case of partial write + _ = os.Remove(dst) return 0, err } diff --git a/test-fixtures/decompress-tar/bad.tar b/test-fixtures/decompress-tar/bad.tar new file mode 100644 index 0000000000000000000000000000000000000000..a7f39e2f6bf40922ee7895b239cb048343858571 GIT binary patch literal 9000 zcmeHJbyU>bw;xbiDM{(>VS=GMB!>oRsUe4Mq#G3}#Sw%{sz`|-ol+9gDL9lkbcYD& z5AW~3>s|M~dw=f_UGJ@RJ!|cC_SxUPPwemc?DGNMuyyoA*m%2n1^|9k1A#yyFc>r7 zYcKro9t08kb$o6Do$F9$Fzl-jL<9x_F@wM`C=>=@2K}fF{c&C&FK;W)^Q~DSk&a$& zu0W)f=Xv)>kALguZgzJ6-Tb@d*OP$$>rl+U+yB2SI8=B5YybdmWy7qc$85y>Tl;DQ z5S$C-Ts+R@>p1UR{ufP7Utjk(x33YXb0PXu7}hsCrHz{l(8}E%i2%BLy7?kptz2yo zfO7|y0qjSN8cu|N3jI0!o*B@#@>fAv*&;jze_S0nhsNLI9}M~v{zX8-Pyn<4_dMud z&Giq*KNQ65;_c{ykOGScgT%x{ARrJBbe?^Pn5Y<-5GKN`p(-z@tDvH4s08%4^7i%w z{!eg~lGBlsQ?(IQaFvI_t?bc5@sFW-M*VG$_oygK}W|NZvAolo{N~_^WuCX+9F=BJU02~~Rs%CP^i?=9J(?(=fLRyxENiDBv$anQfc%3mJT5sPm$KrC7 zj@$A+)kbQM>ULaGzp7lwfBb?hE78=Qh-y-Gv95p!ner!o{`xak}g>rZ`PEHuQ%lJeNo zPRLHBYIEzBjT%tMy!^<#nXEq=8f?nbajHcc%+Q!2xDB~b_gD?$Ybun#2jzuGWYb-V zX>m-;r-jGQoTkUdgFPmSxB@lB-H$g}4r3TXhvj%2&)7O6<)owhh##(TPj|0!J3&*){I!)7Ef3;Z{ME;3 z3Y5mjy6tBS_wcyvM5P(Hg+h$(M$$*a>C5f#5x!N^G5zawJR2i+Nufrn`13N|3xlKv z=GWx#dR*+=1&lZHt)qNPa5E%Vs0p55u1705qry6kCEW6l4x-7zW9<`JjbTh7QKqSN z@x%nhEf=uXd9s63V_zfGH7chBb#~4!Ps6`N7AlkI?FFqhtFKGirYdq)TfI^2UfGT3 zp;@V`(R*k(R1j_CpZ0~V(Sml%DUO7VA8xPIcXFui;AN0;lq&C+DgpyD>W4;C8<)pAsr zc2;_atAcG4E#BU?;FNX1^)JHo$DvA?8rhQ}BA7;*Mq`7hUCz`I@@H38z0x1XM5U2j z5aHB-s|Z7MPwti<_iD&5p`-D-6Agp{d7@=)tXWunL>e;i*7vlajaEE@)TCmvJq}a5 zHOBi)+b&;_as}Gtrlex5l$?WeWpe@3uQO!}J3&Zzyuzg?6ro+camc=|X#x zi8F8=&GM3NF$(t&eimxU&BvnEBF?N6cbG)cl=u(Q-FyistI>=n&d;H1C*w5SdQ#CQ zCrFHZUwnAOB8eucf++i9;}$F_=dtzE^jm(uJz4eTYLU<4Y;(!APkb50qU*Rz+37Siz zq%P=zEED+I8Iz6?W=zI=l&JUN-y<((3UQI6I7NjX;Mkd6j3OYTn8v&3)_n08LwrzKzytQ~Wtr^L}cAEG(c; zU5S*0nRCeLZHmp^h$(LMcSjHWcJq3x*$08w4(jnfSjg9{wdXc@UB9B%5J}Spd~=07f;JSL%n;TsZ&^-*F?}WkQdwW$;`g*Fmkpd!sCejMD@5;`qT81=QITRjQD;QVtP&lf8 zETf#bAuq^k>$4w=QRcmrhs4A+4=F0+4?V>{SqAj^O^&=!BPesoNG$G{2dp)dXu`({ zC3+d^sf-`2@*(V-PgYq;PbgNJwso9yP?ej{WnR#dYtT9*ui*0=S4w3xu{Mo)O%C73 zoa}_sWaCQKk1sJSN0C+Y*1lx)VXO}{fh8xLOyWH%FQtcbhr9-3f3 z*{ksYsufz{oM-la$%N6!4(dYk>1ei^9&7F^dC&`@YgPC*Zwd_(Y;Fg0PqiC=xCeLLXj7kE=W>ekpUkG> z7+s}NE>T{>##W+>3C`C2$;D zb&VY5L>zJ(4A`36*-g4~f8{wj#kegsW%Ikw0&w4LO|4v96SMp)BdwAG(K)m1m5N%a zkM5_r*b&gu3Z~ERbiI{diyF-(V}z={SZQ@eS3BR3M(t6SP!msz%B^rH_ zbrJhuK{{0=N#8}k-+wwZ{fnYyi>>SeCaBo93Hk2e9-e8u63wN}lDm1TFqx1*D18En z4Auu~dfgSAqxgn|MYfAG*0^hl$1-*k6dShyH~POQpIzh@_njWUOe6J?!{3Itnen_DYM18^HF*U56aEWtFjII+U>j5 ziC*nTU0KZ?QnW9$vL}}IkXz#I?Taiv{3E_=hO;9F958tnhI{L5m6gv;{2Vc!ahrQA zvN2?IlvOEHI&&Y03yf+f7CWjur*9@nmTDtk3jymx-e?E)44BRPG%7HlW<;13W@2+J z`nXoPpT?L+2333^*J6{saBZL*SL;nh4>fbGBJZv#K9fjUsXbZ_HSUjGnvGt2h*>?CT19gl&eGT!dtiw@C2s%$cq^71T23lu3bk5T;kGEJ4KInEMt0f12Dnhwb1Lz#>wh85RWm0}3vtg|wAVlQd3hm#hoZVRNia;%ZiW5c@yU&I># zj{GJ9`1SM=xwdjePpjxRp@c6Pb+n=uBTC)!93Z!yojto`{`#Ppymh(5GWY#zLT0b<@~eH7n> z?Sp}=+>F#4^zTvrq7ue@@5*?owie@x2|hIEFW`%)d0<8kKl4QTtLXE5J{g{5q0{1XE>NEDQ+1B9Oxl#?!>6?IDSClYI^}?yypz`n{7GwPa0N6eDGvo6<%_{*AcGJ+~PeL+ds##MY$KqBx&m6WRI}%`JP`9OK7?RHNIZ+&#_JvtEwspM2@KW~hLt^eFc2 zaHpHTU=q3|{qiZ?o!|g+7ih}|IBTF6bSsHg}9 zn|)I&&@;@xsGiN^XVib`x*XNqm5|;$i0X6wlR$HRp-lCt`{3Efu1Dpvp?}|!6X0VN86oR z*r^hPL1O{xOSt$R%Reem%39`I30rkKN)Fa~t5YjlF?@q0aYb7S69~bFz6BV`-*f{o zP$sS5s2lm4fMFFxBQ)NPsq`Q^nOR>mm!gPvQ_I$x({~p-$Zg#yz<`{o57+NnvPXCO zKV!L`b5^AWBx2j(M4M$hRZ0)%UiuR4VxVVsC#7m5<1*H)n0nIdfqRMr3a-TqILtqd`f>tkYq8X3;a>-h^0UAT-<*S(o*GCVRA2P+YEbQO&xLHe+qS z5zV8|bs4mjFZyOqMh2Io9Q)y{-AKSt?OasIe&w#4BZl|cGV&#+j=q_YLBp~*sB7YI zyrWjpT;1F<2LG0wtVt14oOcnYWs(swM1hc)*RmZ-(Y=%T$l|%8oZ*-Z_DyaVWbQRb z%O))sg7q5fdso()RQz@N@*S);?FdDY{`(V@#@?G23dO7u>>J5J$@AtDEDfnybm@yw zr_YAu2g*|M#^I{SIlWEvNQ)&_n<*fPbk7XPnW>&ooIB)4g9Tk?KHAV8^1ykNZQk}T DWpJe- literal 0 HcmV?d00001 From c0ef527809f03855ca2da3b7599ce9edff81ade0 Mon Sep 17 00:00:00 2001 From: Allison Larson Date: Thu, 23 Oct 2025 12:03:08 -0700 Subject: [PATCH 2/2] close file before removing --- get_file_copy.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/get_file_copy.go b/get_file_copy.go index 1bcb4a4bd..95210901d 100644 --- a/get_file_copy.go +++ b/get_file_copy.go @@ -47,7 +47,8 @@ func copyReader(dst string, src io.Reader, fmode, umask os.FileMode, fileSizeLim _, err = io.Copy(dstF, src) if err != nil { - // Remove the file in case of partial write + // Close & remove the file in case of partial write + _ = dstF.Close() _ = os.Remove(dst) return err } @@ -84,7 +85,8 @@ func copyFile(ctx context.Context, dst, src string, disableSymlinks bool, fmode, count, err := Copy(ctx, dstF, srcF) if err != nil { - // Remove the file in case of partial write + // Close & remove the file in case of partial write + _ = dstF.Close() _ = os.Remove(dst) return 0, err }