Skip to content

Commit 11acfc0

Browse files
umbynosumbynos
authored
Use SafeJoin in V2 Tools Install endpoint (#840)
* add testcase and un-export vars * removing test: already tested (better) in `test_main.go` Without the signature field it's falling back to installing from the added index * move test to a different function and add testcase for install endpoint * use SafeJoin on tools Install handler
1 parent 4e5e36e commit 11acfc0

File tree

3 files changed

+103
-79
lines changed

3 files changed

+103
-79
lines changed

‎main_test.go‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -98,26 +98,36 @@ func TestInstallToolV2(t *testing.T) {
9898
responseBody string
9999
}
100100

101-
BossacURL := "http://downloads.arduino.cc/tools/bossac-1.7.0-arduino3-linux64.tar.gz"
102-
BossacChecksum := "SHA-256:1ae54999c1f97234a5c603eb99ad39313b11746a4ca517269a9285afa05f9100"
103-
BossacSignature := "382898a97b5a86edd74208f10107d2fecbf7059ffe9cc856e045266fb4db4e98802728a0859cfdcda1c0b9075ec01e42dbea1f430b813530d5a6ae1766dfbba64c3e689b59758062dc2ab2e32b2a3491dc2b9a80b9cda4ae514fbe0ec5af210111b6896976053ab76bac55bcecfcececa68adfa3299e3cde6b7f117b3552a7d80ca419374bb497e3c3f12b640cf5b20875416b45e662fc6150b99b178f8e41d6982b4c0a255925ea39773683f9aa9201dc5768b6fc857c87ff602b6a93452a541b8ec10ca07f166e61a9e9d91f0a6090bd2038ed4427af6251039fb9fe8eb62ec30d7b0f3df38bc9de7204dec478fb86f8eb3f71543710790ee169dce039d3e0"
101+
bossacURL := "http://downloads.arduino.cc/tools/bossac-1.7.0-arduino3-linux64.tar.gz"
102+
bossacChecksum := "SHA-256:1ae54999c1f97234a5c603eb99ad39313b11746a4ca517269a9285afa05f9100"
103+
bossacSignature := "382898a97b5a86edd74208f10107d2fecbf7059ffe9cc856e045266fb4db4e98802728a0859cfdcda1c0b9075ec01e42dbea1f430b813530d5a6ae1766dfbba64c3e689b59758062dc2ab2e32b2a3491dc2b9a80b9cda4ae514fbe0ec5af210111b6896976053ab76bac55bcecfcececa68adfa3299e3cde6b7f117b3552a7d80ca419374bb497e3c3f12b640cf5b20875416b45e662fc6150b99b178f8e41d6982b4c0a255925ea39773683f9aa9201dc5768b6fc857c87ff602b6a93452a541b8ec10ca07f166e61a9e9d91f0a6090bd2038ed4427af6251039fb9fe8eb62ec30d7b0f3df38bc9de7204dec478fb86f8eb3f71543710790ee169dce039d3e0"
104104
bossacInstallURLOK := tools.ToolPayload{
105105
Name: "bossac",
106106
Version: "1.7.0-arduino3",
107107
Packager: "arduino",
108-
URL: &BossacURL,
109-
Checksum: &BossacChecksum,
110-
Signature: &BossacSignature,
108+
URL: &bossacURL,
109+
Checksum: &bossacChecksum,
110+
Signature: &bossacSignature,
111111
}
112112

113-
WrongSignature := "wr0ngs1gn4tur3"
113+
wrongSignature := "wr0ngs1gn4tur3"
114114
bossacInstallWrongSig := tools.ToolPayload{
115115
Name: "bossac",
116116
Version: "1.7.0-arduino3",
117117
Packager: "arduino",
118-
URL: &BossacURL,
119-
Checksum: &BossacChecksum,
120-
Signature: &WrongSignature,
118+
URL: &bossacURL,
119+
Checksum: &bossacChecksum,
120+
Signature: &wrongSignature,
121+
}
122+
123+
wrongChecksum := "wr0ngch3cksum"
124+
bossacInstallWrongCheck := tools.ToolPayload{
125+
Name: "bossac",
126+
Version: "1.7.0-arduino3",
127+
Packager: "arduino",
128+
URL: &bossacURL,
129+
Checksum: &wrongChecksum,
130+
Signature: &bossacSignature,
121131
}
122132

123133
bossacInstallNoURL := tools.ToolPayload{
@@ -129,6 +139,7 @@ func TestInstallToolV2(t *testing.T) {
129139
tests := []test{
130140
{bossacInstallURLOK, http.StatusOK, "ok"},
131141
{bossacInstallWrongSig, http.StatusInternalServerError, "verification error"},
142+
{bossacInstallWrongCheck, http.StatusInternalServerError, "checksum doesn't match"},
132143
{bossacInstallNoURL, http.StatusBadRequest, "tool not found"}, //because the index is not added
133144
}
134145

‎v2/pkgs/tools.go‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -191,23 +191,28 @@ func (c *Tools) install(ctx context.Context, path, url, checksum string) (*tools
191191
var buffer bytes.Buffer
192192
reader := io.TeeReader(res.Body, &buffer)
193193

194+
safePath, err := utilities.SafeJoin(c.Folder, path)
195+
if err != nil {
196+
return nil, err
197+
}
198+
194199
// Cleanup
195-
err = os.RemoveAll(filepath.Join(c.Folder, path))
200+
err = os.RemoveAll(safePath)
196201
if err != nil {
197202
return nil, err
198203
}
199204

200205
err = extract.Archive(ctx, reader, c.Folder, rename(path))
201206
if err != nil {
202-
os.RemoveAll(path)
207+
os.RemoveAll(safePath)
203208
return nil, err
204209
}
205210

206211
sum := sha256.Sum256(buffer.Bytes())
207212
sumString := "SHA-256:" + hex.EncodeToString(sum[:sha256.Size])
208213

209214
if sumString != checksum {
210-
os.RemoveAll(path)
215+
os.RemoveAll(safePath)
211216
return nil, errors.New("checksum doesn't match")
212217
}
213218

@@ -249,7 +254,11 @@ func writeInstalled(folder, path string) error {
249254
// read installed.json
250255
installed := map[string]string{}
251256

252-
data, err := os.ReadFile(filepath.Join(folder, "installed.json"))
257+
installedFile, err := utilities.SafeJoin(folder, "installed.json")
258+
if err != nil {
259+
return err
260+
}
261+
data, err := os.ReadFile(installedFile)
253262
if err == nil {
254263
err = json.Unmarshal(data, &installed)
255264
if err != nil {
@@ -260,13 +269,17 @@ func writeInstalled(folder, path string) error {
260269
parts := strings.Split(path, string(filepath.Separator))
261270
tool := parts[len(parts)-2]
262271
toolWithVersion := fmt.Sprint(tool, "-", parts[len(parts)-1])
263-
installed[tool] = filepath.Join(folder, path)
264-
installed[toolWithVersion] = filepath.Join(folder, path)
272+
toolFile, err := utilities.SafeJoin(folder, path)
273+
if err != nil {
274+
return err
275+
}
276+
installed[tool] = toolFile
277+
installed[toolWithVersion] = toolFile
265278

266279
data, err = json.Marshal(installed)
267280
if err != nil {
268281
return err
269282
}
270283

271-
return os.WriteFile(filepath.Join(folder, "installed.json"), data, 0644)
284+
return os.WriteFile(installedFile, data, 0644)
272285
}

‎v2/pkgs/tools_test.go‎

Lines changed: 62 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -131,79 +131,79 @@ func TestTools(t *testing.T) {
131131
if len(installed) != 0 {
132132
t.Fatalf("expected %d == %d (%s)", len(installed), 0, "len(installed)")
133133
}
134+
}
134135

135-
// Install a tool by specifying url and checksum
136-
_, err = service.Install(ctx, &tools.ToolPayload{
137-
Packager: "arduino",
138-
Name: "avrdude",
139-
Version: "6.0.1-arduino2",
140-
URL: strpoint(url()),
141-
Checksum: strpoint(checksum()),
142-
})
143-
if err != nil {
144-
t.Fatal(err)
136+
func TestEvilFilename(t *testing.T) {
137+
138+
// Initialize indexes with a temp folder
139+
tmp := t.TempDir()
140+
141+
service := pkgs.Tools{
142+
Folder: tmp,
143+
Indexes: &pkgs.Indexes{
144+
Folder: tmp,
145+
},
145146
}
146147

147-
installed, err = service.Installed(ctx)
148-
if err != nil {
149-
t.Fatal(err)
148+
ctx := context.Background()
149+
150+
type test struct {
151+
fileName string
152+
errBody string
153+
}
154+
155+
evilFileNames := []string{
156+
"/",
157+
"..",
158+
"../",
159+
"../evil.txt",
160+
"../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
161+
"some/path/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
162+
}
163+
if runtime.GOOS == "windows" {
164+
evilFileNames = []string{
165+
"..\\",
166+
"..\\evil.txt",
167+
"..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
168+
"some\\path\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
169+
}
150170
}
151-
if len(installed) != 1 {
152-
t.Fatalf("expected %d == %d (%s)", len(installed), 1, "len(installed)")
171+
tests := []test{}
172+
for _, evilFileName := range evilFileNames {
173+
tests = append(tests, test{fileName: evilFileName,
174+
errBody: "unsafe path join"})
153175
}
154176

155-
t.Run("payload containing evil names", func(t *testing.T) {
156-
evilFileNames := []string{
157-
"/",
158-
"..",
159-
"../",
160-
"../evil.txt",
161-
"../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
162-
"some/path/../../../../../../../../../../../../../../../../../../../../tmp/evil.txt",
163-
}
164-
if runtime.GOOS == "windows" {
165-
evilFileNames = []string{
166-
"..\\",
167-
"..\\evil.txt",
168-
"..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
169-
"some\\path\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\..\\tmp\\evil.txt",
170-
}
171-
}
172-
for _, evilFileName := range evilFileNames {
177+
toolsTemplate := tools.ToolPayload{
178+
// We'll replace the name directly in the test
179+
Checksum: strpoint("SHA-256:1ae54999c1f97234a5c603eb99ad39313b11746a4ca517269a9285afa05f9100"),
180+
Signature: strpoint("382898a97b5a86edd74208f10107d2fecbf7059ffe9cc856e045266fb4db4e98802728a0859cfdcda1c0b9075ec01e42dbea1f430b813530d5a6ae1766dfbba64c3e689b59758062dc2ab2e32b2a3491dc2b9a80b9cda4ae514fbe0ec5af210111b6896976053ab76bac55bcecfcececa68adfa3299e3cde6b7f117b3552a7d80ca419374bb497e3c3f12b640cf5b20875416b45e662fc6150b99b178f8e41d6982b4c0a255925ea39773683f9aa9201dc5768b6fc857c87ff602b6a93452a541b8ec10ca07f166e61a9e9d91f0a6090bd2038ed4427af6251039fb9fe8eb62ec30d7b0f3df38bc9de7204dec478fb86f8eb3f71543710790ee169dce039d3e0"),
181+
URL: strpoint("http://downloads.arduino.cc/tools/bossac-1.7.0-arduino3-linux64.tar.gz"),
182+
}
183+
184+
for _, test := range tests {
185+
t.Run("REMOVE payload containing evil names: "+test.fileName, func(t *testing.T) {
173186
// Here we could inject malicious name also in the Packager and Version field.
174187
// Since the path is made by joining all of these 3 fields, we're using only the Name,
175188
// as it won't change the result and let us keep the test small and readable.
176-
_, err := service.Remove(ctx, &tools.ToolPayload{Name: evilFileName})
177-
require.Error(t, err, evilFileName)
178-
require.ErrorContains(t, err, "unsafe path join")
179-
}
180-
})
189+
_, err := service.Remove(ctx, &tools.ToolPayload{Name: test.fileName})
190+
require.Error(t, err, test)
191+
require.ErrorContains(t, err, test.errBody)
192+
})
193+
}
194+
for _, test := range tests {
195+
toolsTemplate.Name = test.fileName
196+
t.Run("INSTALL payload containing evil names: "+toolsTemplate.Name, func(t *testing.T) {
197+
// Here we could inject malicious name also in the Packager and Version field.
198+
// Since the path is made by joining all of these 3 fields, we're using only the Name,
199+
// as it won't change the result and let us keep the test small and readable.
200+
_, err := service.Install(ctx, &toolsTemplate)
201+
require.Error(t, err, test)
202+
require.ErrorContains(t, err, test.errBody)
203+
})
204+
}
181205
}
182206

183207
func strpoint(s string) *string {
184208
return &s
185209
}
186-
187-
func url() string {
188-
urls := map[string]string{
189-
"linuxamd64": "https://downloads.arduino.cc/tools/avrdude-6.0.1-arduino2-x86_64-pc-linux-gnu.tar.bz2",
190-
"linux386": "https://downloads.arduino.cc/tools/avrdude-6.0.1-arduino2-i686-pc-linux-gnu.tar.bz2",
191-
"darwinamd64": "https://downloads.arduino.cc/tools/avrdude-6.0.1-arduino2-i386-apple-darwin11.tar.bz2",
192-
"windows386": "https://downloads.arduino.cc/tools/avrdude-6.0.1-arduino2-i686-mingw32.zip",
193-
"windowsamd64": "https://downloads.arduino.cc/tools/avrdude-6.0.1-arduino2-i686-mingw32.zip",
194-
}
195-
196-
return urls[runtime.GOOS+runtime.GOARCH]
197-
}
198-
199-
func checksum() string {
200-
checksums := map[string]string{
201-
"linuxamd64": "SHA-256:2489004d1d98177eaf69796760451f89224007c98b39ebb5577a9a34f51425f1",
202-
"linux386": "SHA-256:6f633dd6270ad0d9ef19507bcbf8697b414a15208e4c0f71deec25ef89cdef3f",
203-
"darwinamd64": "SHA-256:71117cce0096dad6c091e2c34eb0b9a3386d3aec7d863d2da733d9e5eac3a6b1",
204-
"windows386": "SHA-256:6c5483800ba753c80893607e30cade8ab77b182808fcc5ea15fa3019c63d76ae",
205-
"windowsamd64": "SHA-256:6c5483800ba753c80893607e30cade8ab77b182808fcc5ea15fa3019c63d76ae",
206-
}
207-
return checksums[runtime.GOOS+runtime.GOARCH]
208-
209-
}

0 commit comments

Comments
 (0)