Skip to content

Commit de07621

Browse files
authored
Fix crash and security defects in four packages (#5080)
## Summary - **libs/sync/gitignore.go**: Add missing `return` after debug log on non-`ErrExist` open failure — prevents nil `*os.File` dereference (crash) - **bundle/direct/dresources/util.go**: Replace bare type assertion `err.(*retries.Err)` with `errors.As` — prevents panic on wrapped/foreign error types during reconciliation - **cmd/labs/unpack/zipball.go**: Guard against empty-zip panic (`zipReader.File[0]` on empty archive), zip slip path traversal (unchecked `filepath.Join`), and untrusted archive mode bits (`zf.Mode()` honored as-is including setuid/world-writable) - **experimental/aitools/cmd/discover_schema.go**: Validate table name parts against `^[A-Za-z_][A-Za-z0-9_]*$` and backtick-quote identifiers — prevents SQL injection via user/agent-supplied table names ## Test plan - [ ] CI passes (unit tests, lint) - [ ] Verify `databricks labs install` still works with valid GitHub zipballs - [ ] Verify `discover-schema` rejects malformed table names like `a.b.c; DROP TABLE x`
1 parent 7748059 commit de07621

4 files changed

Lines changed: 46 additions & 15 deletions

File tree

bundle/direct/dresources/util.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package dresources
22

33
import (
4+
"errors"
45
"fmt"
56
"regexp"
67

@@ -40,14 +41,11 @@ func ParsePostgresName(name string) (PostgresNameComponents, error) {
4041
// This is copied from the retries package of the databricks-sdk-go. It should be made public,
4142
// but for now, I'm copying it here.
4243
func shouldRetry(err error) bool {
43-
if err == nil {
44-
return false
44+
var e *retries.Err
45+
if errors.As(err, &e) {
46+
return !e.Halt
4547
}
46-
e := err.(*retries.Err)
47-
if e == nil {
48-
return false
49-
}
50-
return !e.Halt
48+
return false
5149
}
5250

5351
// collectUpdatePathsWithPrefix extracts field paths from Changes that have action=Update,

cmd/labs/unpack/zipball.go

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package unpack
33
import (
44
"archive/zip"
55
"bytes"
6+
"errors"
67
"fmt"
78
"io"
89
"os"
@@ -25,14 +26,24 @@ func (v GitHubZipball) UnpackTo(libTarget string) error {
2526
if err != nil {
2627
return fmt.Errorf("zip: %w", err)
2728
}
29+
if len(zipReader.File) == 0 {
30+
return errors.New("empty zip archive")
31+
}
2832
// GitHub packages entire repo contents into a top-level folder, e.g. databrickslabs-ucx-2800c6b
2933
rootDirInZIP := zipReader.File[0].Name
3034
for _, zf := range zipReader.File {
3135
if zf.Name == rootDirInZIP {
3236
continue
3337
}
3438
normalizedName := strings.TrimPrefix(zf.Name, rootDirInZIP)
39+
if filepath.IsAbs(normalizedName) || strings.Contains(normalizedName, `\`) {
40+
return fmt.Errorf("invalid zip entry name: %q", zf.Name)
41+
}
3542
targetName := filepath.Join(libTarget, normalizedName)
43+
rel, err := filepath.Rel(libTarget, targetName)
44+
if err != nil || strings.HasPrefix(rel, "..") {
45+
return fmt.Errorf("zip entry escapes target directory: %q", zf.Name)
46+
}
3647
if zf.FileInfo().IsDir() {
3748
err = os.MkdirAll(targetName, ownerRWXworldRX)
3849
if err != nil {
@@ -54,7 +65,7 @@ func (v GitHubZipball) extractFile(zf *zip.File, targetName string) error {
5465
return fmt.Errorf("source: %w", err)
5566
}
5667
defer reader.Close()
57-
writer, err := os.OpenFile(targetName, os.O_CREATE|os.O_RDWR, zf.Mode())
68+
writer, err := os.OpenFile(targetName, os.O_CREATE|os.O_WRONLY, zf.Mode()&0o755)
5869
if err != nil {
5970
return fmt.Errorf("target: %w", err)
6071
}

experimental/aitools/cmd/discover_schema.go

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"errors"
66
"fmt"
7+
"regexp"
78
"strings"
89

910
"github.com/databricks/cli/cmd/root"
@@ -16,6 +17,8 @@ import (
1617
"github.com/spf13/cobra"
1718
)
1819

20+
var sqlIdentifierRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*$`)
21+
1922
func newDiscoverSchemaCmd() *cobra.Command {
2023
cmd := &cobra.Command{
2124
Use: "discover-schema TABLE...",
@@ -37,11 +40,10 @@ For each table, returns:
3740
ctx := cmd.Context()
3841
w := cmdctx.WorkspaceClient(ctx)
3942

40-
// validate table names
43+
// validate table names: each part must be a safe SQL identifier
4144
for _, table := range args {
42-
parts := strings.Split(table, ".")
43-
if len(parts) != 3 {
44-
return fmt.Errorf("invalid table format %q: expected CATALOG.SCHEMA.TABLE", table)
45+
if _, err := quoteTableName(table); err != nil {
46+
return err
4547
}
4648
}
4749

@@ -94,8 +96,13 @@ For each table, returns:
9496
func discoverTable(ctx context.Context, w *databricks.WorkspaceClient, warehouseID, table string) (string, error) {
9597
var sb strings.Builder
9698

99+
quoted, err := quoteTableName(table)
100+
if err != nil {
101+
return "", err
102+
}
103+
97104
// 1. describe table - get columns and types
98-
describeSQL := "DESCRIBE TABLE " + table
105+
describeSQL := "DESCRIBE TABLE " + quoted
99106
descResp, err := executeSQL(ctx, w, warehouseID, describeSQL)
100107
if err != nil {
101108
return "", fmt.Errorf("describe table: %w", err)
@@ -112,7 +119,7 @@ func discoverTable(ctx context.Context, w *databricks.WorkspaceClient, warehouse
112119
}
113120

114121
// 2. sample data (5 rows)
115-
sampleSQL := fmt.Sprintf("SELECT * FROM %s LIMIT 5", table)
122+
sampleSQL := fmt.Sprintf("SELECT * FROM %s LIMIT 5", quoted)
116123
sampleResp, err := executeSQL(ctx, w, warehouseID, sampleSQL)
117124
if err != nil {
118125
fmt.Fprintf(&sb, "\nSAMPLE DATA: Error - %v\n", err)
@@ -127,7 +134,7 @@ func discoverTable(ctx context.Context, w *databricks.WorkspaceClient, warehouse
127134
nullCountExprs[i] = fmt.Sprintf("SUM(CASE WHEN `%s` IS NULL THEN 1 ELSE 0 END) AS `%s_nulls`", col, col)
128135
}
129136
nullSQL := fmt.Sprintf("SELECT COUNT(*) AS total_rows, %s FROM %s",
130-
strings.Join(nullCountExprs, ", "), table)
137+
strings.Join(nullCountExprs, ", "), quoted)
131138

132139
nullResp, err := executeSQL(ctx, w, warehouseID, nullSQL)
133140
if err != nil {
@@ -231,3 +238,17 @@ func formatNullCounts(resp *dbsql.StatementResponse, columns []string) string {
231238

232239
return sb.String()
233240
}
241+
242+
// quoteTableName validates and backtick-quotes a CATALOG.SCHEMA.TABLE identifier.
243+
func quoteTableName(table string) (string, error) {
244+
parts := strings.Split(table, ".")
245+
if len(parts) != 3 {
246+
return "", fmt.Errorf("invalid table format %q: expected CATALOG.SCHEMA.TABLE", table)
247+
}
248+
for _, part := range parts {
249+
if !sqlIdentifierRe.MatchString(part) {
250+
return "", fmt.Errorf("invalid SQL identifier %q in table name %q", part, table)
251+
}
252+
}
253+
return fmt.Sprintf("`%s`.`%s`.`%s`", parts[0], parts[1], parts[2]), nil
254+
}

libs/sync/gitignore.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ func WriteGitIgnore(ctx context.Context, dir string) {
1818
return
1919
}
2020
log.Debugf(ctx, "Failed to create %s: %s", gitignorePath, err)
21+
return
2122
}
2223

2324
defer file.Close()

0 commit comments

Comments
 (0)