Skip to content

Commit d96c5a9

Browse files
authored
Merge pull request #1727 from ClickHouse/kavirajk/fix-formatTime-sql-escaping
bug: Fix `formatTime` escaping
2 parents b4fd33b + 997700e commit d96c5a9

File tree

2 files changed

+65
-3
lines changed

2 files changed

+65
-3
lines changed

bind.go

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package clickhouse
22

33
import (
44
std_driver "database/sql/driver"
5+
"errors"
56
"fmt"
67
"reflect"
78
"regexp"
@@ -12,6 +13,10 @@ import (
1213
"github.com/ClickHouse/clickhouse-go/v2/lib/driver"
1314
)
1415

16+
var (
17+
ErrInvalidTimezone = errors.New("invalid timezone value")
18+
)
19+
1520
func Named(name string, value any) driver.NamedValue {
1621
return driver.NamedValue{
1722
Name: name,
@@ -228,7 +233,9 @@ func bindNamed(tz *time.Location, query string, args ...any) (_ string, err erro
228233
}
229234

230235
func formatTime(tz *time.Location, scale TimeUnit, value time.Time) (string, error) {
231-
switch value.Location().String() {
236+
locVal := value.Location().String()
237+
238+
switch locVal {
232239
case "Local", "":
233240
// It's required to pass timestamp as string due to decimal overflow for higher precision,
234241
// but zero-value string "toDateTime('0')" will be not parsed by ClickHouse.
@@ -252,10 +259,17 @@ func formatTime(tz *time.Location, scale TimeUnit, value time.Time) (string, err
252259
}
253260
return fmt.Sprintf("toDateTime64('%s', %d)", value.Format(fmt.Sprintf("2006-01-02 15:04:05.%0*d", int(scale*3), 0)), int(scale*3)), nil
254261
}
262+
263+
// Escape the timezone string (timezone may contain malicious SQL query)
264+
escapedTimezone := stringQuoteReplacer.Replace(locVal)
265+
if locVal != escapedTimezone {
266+
return "", fmt.Errorf("%w: %q", ErrInvalidTimezone, locVal)
267+
}
268+
255269
if scale == Seconds {
256-
return fmt.Sprintf("toDateTime('%s', '%s')", value.Format("2006-01-02 15:04:05"), value.Location().String()), nil
270+
return fmt.Sprintf("toDateTime('%s', '%s')", value.Format("2006-01-02 15:04:05"), escapedTimezone), nil
257271
}
258-
return fmt.Sprintf("toDateTime64('%s', %d, '%s')", value.Format(fmt.Sprintf("2006-01-02 15:04:05.%0*d", int(scale*3), 0)), int(scale*3), value.Location().String()), nil
272+
return fmt.Sprintf("toDateTime64('%s', %d, '%s')", value.Format(fmt.Sprintf("2006-01-02 15:04:05.%0*d", int(scale*3), 0)), int(scale*3), escapedTimezone), nil
259273
}
260274

261275
var stringQuoteReplacer = strings.NewReplacer(`\`, `\\`, `'`, `\'`)

bind_test.go

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,54 @@ func TestFormatMap(t *testing.T) {
330330
assert.Equal(t, "map('a', 1)", val)
331331
}
332332

333+
func TestTimezoneSQLEscaping(t *testing.T) {
334+
t.Run("prevent SQL injection via timezone name", func(t *testing.T) {
335+
maliciousLoc := time.FixedZone("UTC') UNION ALL SELECT 1,2,3 --", 0)
336+
time.LoadLocation(maliciousLoc.String())
337+
maliciousTime := time.Now().In(maliciousLoc)
338+
339+
val, err := format(time.UTC, Seconds, maliciousTime)
340+
require.Error(t, err)
341+
assert.Equal(t, "", val)
342+
assert.ErrorIs(t, err, ErrInvalidTimezone)
343+
})
344+
345+
t.Run("prevent SQL injection via timezone name with milliseconds", func(t *testing.T) {
346+
maliciousLoc := time.FixedZone("America/New_York'); DROP TABLE users; --", 0)
347+
maliciousTime := time.Now().In(maliciousLoc)
348+
349+
val, err := format(time.UTC, MilliSeconds, maliciousTime)
350+
require.Error(t, err)
351+
assert.Equal(t, "", val)
352+
assert.ErrorIs(t, err, ErrInvalidTimezone)
353+
})
354+
355+
t.Run("prevent SQL injection via timezone with backslashes", func(t *testing.T) {
356+
maliciousLoc := time.FixedZone(`UTC\' OR 1=1 --`, 0)
357+
maliciousTime := time.Now().In(maliciousLoc)
358+
359+
val, err := format(time.UTC, Seconds, maliciousTime)
360+
// require.NoError(t, err)
361+
require.Error(t, err)
362+
assert.Equal(t, "", val)
363+
assert.ErrorIs(t, err, ErrInvalidTimezone)
364+
})
365+
366+
t.Run("normal timezone names remain unaffected", func(t *testing.T) {
367+
// Test that normal, safe timezone names still work correctly
368+
normalLoc := time.FixedZone("America/New_York", -5*3600)
369+
normalTime := time.Now().In(normalLoc)
370+
371+
val, err := format(time.UTC, Seconds, normalTime)
372+
require.NoError(t, err)
373+
374+
// Should contain the timezone name without any escaping
375+
assert.Contains(t, val, "'America/New_York'")
376+
assert.NotContains(t, val, `\'`, "Normal timezone names should not have escaped quotes")
377+
assert.Contains(t, val, "toDateTime(")
378+
})
379+
}
380+
333381
// a simple (non thread safe) ordered map, implementing the column.OrderedMap interface
334382
type OrderedMap struct {
335383
keys []any

0 commit comments

Comments
 (0)