[Search] Fix CodeQL warnings (#101364)
* Fix CodeQL warnings * Also validate if path is a file above the safe dir * Lint * [REVIEW] reduce possibility of exploits * Comment * Remove test scenario
This commit is contained in:
@@ -5,6 +5,7 @@ import (
|
|||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
"log/slog"
|
"log/slog"
|
||||||
|
"math"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"slices"
|
"slices"
|
||||||
@@ -173,6 +174,9 @@ func (b *bleveBackend) BuildIndex(ctx context.Context,
|
|||||||
fname = b.start.Format("tmp-20060102-150405")
|
fname = b.start.Format("tmp-20060102-150405")
|
||||||
}
|
}
|
||||||
dir := filepath.Join(resourceDir, fname)
|
dir := filepath.Join(resourceDir, fname)
|
||||||
|
if !isValidPath(dir, b.opts.Root) {
|
||||||
|
b.log.Error("Directory is not valid", "directory", dir)
|
||||||
|
}
|
||||||
if resourceVersion > 0 {
|
if resourceVersion > 0 {
|
||||||
info, _ := os.Stat(dir)
|
info, _ := os.Stat(dir)
|
||||||
if info != nil && info.IsDir() {
|
if info != nil && info.IsDir() {
|
||||||
@@ -263,6 +267,9 @@ func (b *bleveBackend) cleanOldIndexes(dir string, skip string) {
|
|||||||
for _, file := range files {
|
for _, file := range files {
|
||||||
if file.IsDir() && file.Name() != skip {
|
if file.IsDir() && file.Name() != skip {
|
||||||
fpath := filepath.Join(dir, file.Name())
|
fpath := filepath.Join(dir, file.Name())
|
||||||
|
if !isValidPath(dir, b.opts.Root) {
|
||||||
|
b.log.Error("Path is not valid", "directory", fpath, "error", err)
|
||||||
|
}
|
||||||
err = os.RemoveAll(fpath)
|
err = os.RemoveAll(fpath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
b.log.Error("Unable to remove old index folder", "directory", fpath, "error", err)
|
b.log.Error("Unable to remove old index folder", "directory", fpath, "error", err)
|
||||||
@@ -273,6 +280,21 @@ func (b *bleveBackend) cleanOldIndexes(dir string, skip string) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// isValidPath does a sanity check in case it tries to access a different dir
|
||||||
|
func isValidPath(path, safeDir string) bool {
|
||||||
|
if path == "" || safeDir == "" {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
cleanPath := filepath.Clean(path)
|
||||||
|
cleanSafeDir := filepath.Clean(safeDir)
|
||||||
|
|
||||||
|
rel, err := filepath.Rel(cleanSafeDir, cleanPath)
|
||||||
|
if err != nil {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
return !strings.HasPrefix(rel, "..") && !strings.Contains(rel, "\\")
|
||||||
|
}
|
||||||
|
|
||||||
// TotalDocs returns the total number of documents across all indices
|
// TotalDocs returns the total number of documents across all indices
|
||||||
func (b *bleveBackend) TotalDocs() int64 {
|
func (b *bleveBackend) TotalDocs() int64 {
|
||||||
var totalDocs int64
|
var totalDocs int64
|
||||||
@@ -630,10 +652,19 @@ func (b *bleveIndex) toBleveSearchRequest(ctx context.Context, req *resource.Res
|
|||||||
fields = append(fields, f)
|
fields = append(fields, f)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
size, err := safeInt64ToInt(req.Limit)
|
||||||
|
if err != nil {
|
||||||
|
return nil, resource.AsErrorResult(err)
|
||||||
|
}
|
||||||
|
offset, err := safeInt64ToInt(req.Offset)
|
||||||
|
if err != nil {
|
||||||
|
return nil, resource.AsErrorResult(err)
|
||||||
|
}
|
||||||
|
|
||||||
searchrequest := &bleve.SearchRequest{
|
searchrequest := &bleve.SearchRequest{
|
||||||
Fields: fields,
|
Fields: fields,
|
||||||
Size: int(req.Limit),
|
Size: size,
|
||||||
From: int(req.Offset),
|
From: offset,
|
||||||
Explain: req.Explain,
|
Explain: req.Explain,
|
||||||
Facets: facets,
|
Facets: facets,
|
||||||
}
|
}
|
||||||
@@ -768,6 +799,13 @@ func (b *bleveIndex) toBleveSearchRequest(ctx context.Context, req *resource.Res
|
|||||||
return searchrequest, nil
|
return searchrequest, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func safeInt64ToInt(i64 int64) (int, error) {
|
||||||
|
if i64 > math.MaxInt32 || i64 < math.MinInt32 {
|
||||||
|
return 0, fmt.Errorf("int64 value %d overflows int", i64)
|
||||||
|
}
|
||||||
|
return int(i64), nil
|
||||||
|
}
|
||||||
|
|
||||||
func getSortFields(req *resource.ResourceSearchRequest) []string {
|
func getSortFields(req *resource.ResourceSearchRequest) []string {
|
||||||
sorting := []string{}
|
sorting := []string{}
|
||||||
for _, sort := range req.SortBy {
|
for _, sort := range req.SortBy {
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"math"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"testing"
|
"testing"
|
||||||
@@ -596,3 +597,104 @@ func (nc StubAccessClient) Write(ctx context.Context, req *authzextv1.WriteReque
|
|||||||
func (nc StubAccessClient) BatchCheck(ctx context.Context, req *authzextv1.BatchCheckRequest) (*authzextv1.BatchCheckResponse, error) {
|
func (nc StubAccessClient) BatchCheck(ctx context.Context, req *authzextv1.BatchCheckRequest) (*authzextv1.BatchCheckResponse, error) {
|
||||||
return nil, nil
|
return nil, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestSafeInt64ToInt(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
input int64
|
||||||
|
want int
|
||||||
|
wantErr bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "Valid int64 within int range",
|
||||||
|
input: 42,
|
||||||
|
want: 42,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "Overflow int64 value",
|
||||||
|
input: math.MaxInt64,
|
||||||
|
want: 0,
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "Underflow int64 value",
|
||||||
|
input: math.MinInt64,
|
||||||
|
want: 0,
|
||||||
|
wantErr: true,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
got, err := safeInt64ToInt(tt.input)
|
||||||
|
if tt.wantErr {
|
||||||
|
require.Error(t, err)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
require.Equal(t, tt.want, got)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func Test_isValidPath(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
dir string
|
||||||
|
safeDir string
|
||||||
|
want bool
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "valid path",
|
||||||
|
dir: "/path/to/my-file/",
|
||||||
|
safeDir: "/path/to/",
|
||||||
|
want: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "valid path without trailing slash",
|
||||||
|
dir: "/path/to/my-file",
|
||||||
|
safeDir: "/path/to",
|
||||||
|
want: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "path with double slashes",
|
||||||
|
dir: "/path//to//my-file/",
|
||||||
|
safeDir: "/path/to/",
|
||||||
|
want: true,
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "invalid path: ..",
|
||||||
|
dir: "/path/../above/",
|
||||||
|
safeDir: "/path/to/",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "invalid path: \\",
|
||||||
|
dir: "\\path/to",
|
||||||
|
safeDir: "/path/to/",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "invalid path: not under safe dir",
|
||||||
|
dir: "/path/to.txt",
|
||||||
|
safeDir: "/path/to/",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "invalid path: empty paths",
|
||||||
|
dir: "",
|
||||||
|
safeDir: "/path/to/",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "invalid path: different path",
|
||||||
|
dir: "/other/path/to/my-file/",
|
||||||
|
safeDir: "/Some/other/path",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "invalid path: empty safe path",
|
||||||
|
dir: "/path/to/",
|
||||||
|
safeDir: "",
|
||||||
|
},
|
||||||
|
}
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
require.Equal(t, tt.want, isValidPath(tt.dir, tt.safeDir))
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user