diff --git a/pkg/caldav/caldav.go b/pkg/caldav/caldav.go index d93f9ec91..d89f87636 100644 --- a/pkg/caldav/caldav.go +++ b/pkg/caldav/caldav.go @@ -17,7 +17,6 @@ package caldav import ( - "regexp" "strconv" "strings" "time" @@ -30,6 +29,19 @@ import ( // DateFormat is the caldav date format const DateFormat = `20060102T150405` +// escapeICalText escapes a string for use inside an RFC 5545 ยง3.3.11 TEXT +// property value. Backslash MUST be replaced first so the backslashes this +// function inserts are not themselves re-escaped. +func escapeICalText(s string) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + s = strings.ReplaceAll(s, "\r", "\n") + s = strings.ReplaceAll(s, `\`, `\\`) + s = strings.ReplaceAll(s, "\n", `\n`) + s = strings.ReplaceAll(s, `;`, `\;`) + s = strings.ReplaceAll(s, `,`, `\,`) + return s +} + // Todo holds a single VTODO type Todo struct { // Required @@ -77,16 +89,25 @@ type Config struct { Color string } +// getCaldavColor sanitizes color at the output boundary (non-hex chars +// dropped) so upstream HexColor validation gaps cannot lead to iCal +// property injection via CR/LF in a crafted color string. func getCaldavColor(color string) (caldavcolor string) { + color = strings.TrimPrefix(color, "#") + + var b strings.Builder + b.Grow(len(color)) + for _, r := range color { + if (r >= '0' && r <= '9') || (r >= 'a' && r <= 'f') || (r >= 'A' && r <= 'F') { + b.WriteRune(r) + } + } + color = b.String() if color == "" { return "" } - if !strings.HasPrefix(color, "#") { - color = "#" + color - } - - color += "FF" + color = "#" + color + "FF" return ` X-APPLE-CALENDAR-COLOR:` + color + ` @@ -131,7 +152,7 @@ func ParseTodos(config *Config, todos []*Todo) (caldavtodos string) { caldavtodos = `BEGIN:VCALENDAR VERSION:2.0 X-PUBLISHED-TTL:PT4H -X-WR-CALNAME:` + config.Name + ` +X-WR-CALNAME:` + escapeICalText(config.Name) + ` PRODID:-//` + config.ProdID + `//EN` + getCaldavColor(config.Color) for _, t := range todos { @@ -141,9 +162,9 @@ PRODID:-//` + config.ProdID + `//EN` + getCaldavColor(config.Color) caldavtodos += ` BEGIN:VTODO -UID:` + t.UID + ` +UID:` + escapeICalText(t.UID) + ` DTSTAMP:` + makeCalDavTimeFromTimeStamp(t.Timestamp) + ` -SUMMARY:` + t.Summary + getCaldavColor(t.Color) +SUMMARY:` + escapeICalText(t.Summary) + getCaldavColor(t.Color) if t.Start.Unix() > 0 { caldavtodos += ` @@ -158,10 +179,8 @@ DURATION:PT` + formatDuration(t.Duration) DTEND:` + makeCalDavTimeFromTimeStamp(t.End) } if t.Description != "" { - re := regexp.MustCompile(`\r?\n`) - formattedDescription := re.ReplaceAllString(t.Description, "\\n") caldavtodos += ` -DESCRIPTION:` + formattedDescription +DESCRIPTION:` + escapeICalText(t.Description) } if t.Completed.Unix() > 0 { caldavtodos += ` @@ -170,7 +189,7 @@ STATUS:COMPLETED` } if t.Organizer != nil { caldavtodos += ` -ORGANIZER;CN=:` + t.Organizer.Username +ORGANIZER;CN=:` + escapeICalText(t.Organizer.Username) } if t.DueDate.Unix() > 0 { @@ -200,8 +219,12 @@ RRULE:FREQ=` + freq + `;INTERVAL=` + strconv.FormatInt(interval, 10) } if len(t.Categories) > 0 { + escapedCategories := make([]string, len(t.Categories)) + for i, c := range t.Categories { + escapedCategories[i] = escapeICalText(c) + } caldavtodos += ` -CATEGORIES:` + strings.Join(t.Categories, ",") +CATEGORIES:` + strings.Join(escapedCategories, ",") } caldavtodos += ` @@ -239,7 +262,7 @@ TRIGGER;VALUE=DATE-TIME:` + makeCalDavTimeFromTimeStamp(a.Time) } caldavalarms += ` ACTION:DISPLAY -DESCRIPTION:` + a.Description + ` +DESCRIPTION:` + escapeICalText(a.Description) + ` END:VALARM` } return caldavalarms @@ -280,7 +303,7 @@ RELATED-TO;RELTYPE=CHILD:` RELATED-TO:` } - caldavrelatedtos += r.UID + caldavrelatedtos += escapeICalText(r.UID) } return caldavrelatedtos diff --git a/pkg/caldav/caldav_test.go b/pkg/caldav/caldav_test.go index d95da57e2..7ced0b5e4 100644 --- a/pkg/caldav/caldav_test.go +++ b/pkg/caldav/caldav_test.go @@ -23,6 +23,7 @@ import ( "code.vikunja.io/api/pkg/models" "code.vikunja.io/api/pkg/config" + "code.vikunja.io/api/pkg/user" "github.com/stretchr/testify/assert" ) @@ -365,6 +366,66 @@ LAST-MODIFIED:00010101T000000Z RELATED-TO;RELTYPE=PARENT:parentuid RELATED-TO;RELTYPE=CHILD:subtaskuid END:VTODO +END:VCALENDAR`, + }, + { + name: "GHSA-2g7h-7rqr-9p4r: CRLF injection in Summary is escaped", + args: args{ + config: &Config{ + Name: "test", + ProdID: "RandomProdID which is not random", + }, + todos: []*Todo{ + { + Summary: "Meeting\r\nATTACH:https://evil.com/malware.exe\r\nX-INJECTED:pwned", + UID: "randommduid", + Timestamp: time.Unix(1543626724, 0).In(config.GetTimeZone()), + }, + }, + }, + wantCaldavtasks: `BEGIN:VCALENDAR +VERSION:2.0 +X-PUBLISHED-TTL:PT4H +X-WR-CALNAME:test +PRODID:-//RandomProdID which is not random//EN +BEGIN:VTODO +UID:randommduid +DTSTAMP:20181201T011204Z +SUMMARY:Meeting\nATTACH:https://evil.com/malware.exe\nX-INJECTED:pwned +LAST-MODIFIED:00010101T000000Z +END:VTODO +END:VCALENDAR`, + }, + { + name: "GHSA-2g7h-7rqr-9p4r: semicolons and commas in Summary are escaped", + args: args{ + config: &Config{ + Name: "te;st,ed", + ProdID: "RandomProdID which is not random", + }, + todos: []*Todo{ + { + Summary: "a;b,c\\d", + UID: "randommduid", + Timestamp: time.Unix(1543626724, 0).In(config.GetTimeZone()), + Categories: []string{"lab;el1", "lab,el2"}, + Organizer: &user.User{Username: "al;ic,e"}, + }, + }, + }, + wantCaldavtasks: `BEGIN:VCALENDAR +VERSION:2.0 +X-PUBLISHED-TTL:PT4H +X-WR-CALNAME:te\;st\,ed +PRODID:-//RandomProdID which is not random//EN +BEGIN:VTODO +UID:randommduid +DTSTAMP:20181201T011204Z +SUMMARY:a\;b\,c\\d +ORGANIZER;CN=:al\;ic\,e +CATEGORIES:lab\;el1,lab\,el2 +LAST-MODIFIED:00010101T000000Z +END:VTODO END:VCALENDAR`, }, } @@ -376,3 +437,54 @@ END:VCALENDAR`, }) } } + +func TestGetCaldavColor(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"empty", "", ""}, + {"plain hex", "ff8800", "\nX-APPLE-CALENDAR-COLOR:#ff8800FF\nX-OUTLOOK-COLOR:#ff8800FF\nX-FUNAMBOL-COLOR:#ff8800FF\nCOLOR:#ff8800FF"}, + {"leading hash", "#ff8800", "\nX-APPLE-CALENDAR-COLOR:#ff8800FF\nX-OUTLOOK-COLOR:#ff8800FF\nX-FUNAMBOL-COLOR:#ff8800FF\nCOLOR:#ff8800FF"}, + {"mixed case", "AaBbCc", "\nX-APPLE-CALENDAR-COLOR:#AaBbCcFF\nX-OUTLOOK-COLOR:#AaBbCcFF\nX-FUNAMBOL-COLOR:#AaBbCcFF\nCOLOR:#AaBbCcFF"}, + {"CRLF injection stripped", "a\r\nB", "\nX-APPLE-CALENDAR-COLOR:#aBFF\nX-OUTLOOK-COLOR:#aBFF\nX-FUNAMBOL-COLOR:#aBFF\nCOLOR:#aBFF"}, + {"property injection stripped", "ff\r\nATTACH:https://evil.com", "\nX-APPLE-CALENDAR-COLOR:#ffAACecFF\nX-OUTLOOK-COLOR:#ffAACecFF\nX-FUNAMBOL-COLOR:#ffAACecFF\nCOLOR:#ffAACecFF"}, + {"non-hex chars dropped entirely", "zz!@#", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := getCaldavColor(tt.in) + assert.Equal(t, tt.want, got) + // No output may ever contain CR or LF inside a property value. + assert.NotContains(t, got, "\r") + }) + } +} + +func TestEscapeICalText(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"empty", "", ""}, + {"plain ASCII", "Hello World", "Hello World"}, + {"backslash", `a\b`, `a\\b`}, + {"semicolon", "a;b", `a\;b`}, + {"comma", "a,b", `a\,b`}, + {"newline LF", "a\nb", `a\nb`}, + {"carriage return LF pair", "a\r\nb", `a\nb`}, + {"lone carriage return", "a\rb", `a\nb`}, + {"multiple specials", `a\;b,c` + "\n" + "d", `a\\\;b\,c\nd`}, + {"backslash-n in source stays separate", `a\nb`, `a\\nb`}, + {"advisory PoC (CRLF + ATTACH)", "Meeting\r\nATTACH:https://evil.com/malware.exe", `Meeting\nATTACH:https://evil.com/malware.exe`}, + {"colon is NOT escaped (not a TEXT special)", "a:b", "a:b"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := escapeICalText(tt.in) + assert.Equal(t, tt.want, got) + }) + } +}