Skip to content

Commit 69b6888

Browse files
authored
Strip newlines from archive entry paths in diff headers (#8)
Archive entry names can contain literal newline characters, which when interpolated into unified diff `---`/`+++` headers produce attacker-controlled diff control lines. Strips `\n` and `\r` from paths before emitting diff headers in `generateSimpleDiff` and `generateAddedDiff`.
1 parent 96dc088 commit 69b6888

2 files changed

Lines changed: 66 additions & 4 deletions

File tree

diff/diff.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -208,10 +208,10 @@ func generateSimpleDiff(path string, oldContent, newContent []byte) (string, int
208208
oldLines := strings.Split(string(oldContent), "\n")
209209
newLines := strings.Split(string(newContent), "\n")
210210

211-
// Simple line-by-line comparison (can be improved with Myers algorithm)
212211
var buf strings.Builder
213-
fmt.Fprintf(&buf, "--- a/%s\n", path)
214-
fmt.Fprintf(&buf, "+++ b/%s\n", path)
212+
safePath := sanitizeDiffPath(path)
213+
fmt.Fprintf(&buf, "--- a/%s\n", safePath)
214+
fmt.Fprintf(&buf, "+++ b/%s\n", safePath)
215215

216216
linesAdded := 0
217217
linesDeleted := 0
@@ -301,8 +301,9 @@ func generateSimpleDiff(path string, oldContent, newContent []byte) (string, int
301301
// generateAddedDiff generates a diff for a newly added file.
302302
func generateAddedDiff(path string, content []byte) string {
303303
var buf strings.Builder
304+
safePath := sanitizeDiffPath(path)
304305
buf.WriteString("--- /dev/null\n")
305-
fmt.Fprintf(&buf, "+++ b/%s\n", path)
306+
fmt.Fprintf(&buf, "+++ b/%s\n", safePath)
306307

307308
lines := bytes.Split(content, []byte("\n"))
308309
fmt.Fprintf(&buf, "@@ -0,0 +1,%d @@\n", len(lines))
@@ -314,6 +315,12 @@ func generateAddedDiff(path string, content []byte) string {
314315
return buf.String()
315316
}
316317

318+
var diffPathReplacer = strings.NewReplacer("\n", "", "\r", "")
319+
320+
func sanitizeDiffPath(path string) string {
321+
return diffPathReplacer.Replace(path)
322+
}
323+
317324
// countLines counts the number of lines in content.
318325
func countLines(content []byte) int {
319326
scanner := bufio.NewScanner(bytes.NewReader(content))

diff/diff_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,3 +252,58 @@ func TestCompareBinaryFiles(t *testing.T) {
252252
t.Error("binary files should not have diff content")
253253
}
254254
}
255+
256+
func TestSanitizeDiffPath(t *testing.T) {
257+
tests := []struct {
258+
input string
259+
want string
260+
}{
261+
{"normal/path.go", "normal/path.go"},
262+
{"has\nnewline", "hasnewline"},
263+
{"has\r\ncrlf", "hascrlf"},
264+
{"multi\n\n\nnewlines", "multinewlines"},
265+
{"", ""},
266+
}
267+
268+
for _, tt := range tests {
269+
got := sanitizeDiffPath(tt.input)
270+
if got != tt.want {
271+
t.Errorf("sanitizeDiffPath(%q) = %q, want %q", tt.input, got, tt.want)
272+
}
273+
}
274+
}
275+
276+
func TestDiffHeaderInjectionBlocked(t *testing.T) {
277+
injectedPath := "evil.txt\n+++ b/etc/passwd\n@@ -0,0 +1 @@\n+root:x:0:0"
278+
content := []byte("hello\n")
279+
280+
diff := generateAddedDiff(injectedPath, content)
281+
282+
headerCount := 0
283+
for _, line := range strings.Split(diff, "\n") {
284+
if strings.HasPrefix(line, "+++ ") {
285+
headerCount++
286+
}
287+
}
288+
if headerCount != 1 {
289+
t.Errorf("expected 1 +++ header, got %d; newlines in path were not stripped", headerCount)
290+
}
291+
}
292+
293+
func TestUnifiedDiffHeaderInjectionBlocked(t *testing.T) {
294+
injectedPath := "evil.txt\n--- a/etc/shadow"
295+
oldContent := []byte("old\n")
296+
newContent := []byte("new\n")
297+
298+
diff, _, _ := generateUnifiedDiff(injectedPath, oldContent, newContent)
299+
300+
minusCount := 0
301+
for _, line := range strings.Split(diff, "\n") {
302+
if strings.HasPrefix(line, "--- ") {
303+
minusCount++
304+
}
305+
}
306+
if minusCount != 1 {
307+
t.Errorf("expected 1 --- header, got %d; newlines in path were not stripped", minusCount)
308+
}
309+
}

0 commit comments

Comments
 (0)