JOJ/JOJ3
25
6
Fork 1
Code Issues 6 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix: apply security patches for configuration file and local executor #97

Closed
蔡雨翔524370910013 wants to merge 1 commits from arthurcai/JOJ3:master into master
pull from: arthurcai/JOJ3:master
merge into: JOJ:master
Conversation 0 Commits 1 Files Changed 2 +22 -2
2 changed files with 22 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0a2d783dcd - Show all commits

14
cmd/joj3/conf/conf.go
View File

@ -13,6 +13,7 @@ import (
"path/filepath"
"regexp"
"strings"
"syscall"
"github.com/go-git/go-git/v5"
"github.com/koding/multiconfig"
@ -183,6 +184,19 @@ func GetConfPath(confRoot, confName, fallbackConfName, msg, tag string) (
return confPath, confStat, conventionalCommit, err
}
}
// Check file ownership
if stat, ok := confStat.Sys().(*syscall.Stat_t); ok {
uid := int(stat.Uid)
currentUid := os.Getuid()
if uid != 0 && uid != currentUid {
张泊明518370910136 commented 2026-03-13 08:41:39 +08:00
Review
Copy Link

Will the conf file be owned by root?

Will the conf file be owned by root?
manuel commented 2026-03-13 08:44:34 +08:00
Review
Copy Link

i was wondering the same: we have student and tt. i think no other user (including root) is involved.

i was wondering the same: we have `student` and `tt`. i think no other user (including `root`) is involved.
蔡雨翔524370910013 commented 2026-03-13 12:12:04 +08:00
Review
Copy Link

Then uid != 0 is not needed.

Then uid != 0 is not needed.
err = fmt.Errorf("insecure configuration file: owned by uid %d, expected 0 or %d", uid, currentUid)
slog.Error("insecure conf file", "path", confPath, "uid", uid, "expected_uid", currentUid)
return confPath, confStat, conventionalCommit, err
}
} else {
slog.Warn("could not determine file ownership, proceeding with caution", "path", confPath)
张泊明518370910136 commented 2026-03-13 08:39:53 +08:00
Review
Copy Link

What if we just terminate it here?

What if we just terminate it here?
manuel commented 2026-03-13 08:42:08 +08:00
Review
Copy Link

maybe end + write logs to raise an alert on grafana?

maybe end + write logs to raise an alert on grafana?
}
return confPath, confStat, conventionalCommit, err
}

10
internal/executor/local/meta.go
View File

@ -3,12 +3,18 @@
// used for passing run time parameters.
package local
import "github.com/joint-online-judge/JOJ3/internal/stage"
import (
"os"
"github.com/joint-online-judge/JOJ3/internal/stage"
)
var name = "local"
type Local struct{}
func init() {
stage.RegisterExecutor(name, &Local{})
if os.Getenv("JOJ3_ENABLE_LOCAL_EXECUTOR") == "true" {
张泊明518370910136 commented 2026-03-13 08:40:25 +08:00
Review
Copy Link

Where is it used?

Where is it used?
蔡雨翔524370910013 commented 2026-03-13 12:11:22 +08:00
Review
Copy Link

Just thinking that the Local executor is unsafe if conf.go is bypassed, so you can add an env for the students' executor so that it doesn't run the local executor, while other usages can still be fulfilled.

Just thinking that the Local executor is unsafe if conf.go is bypassed, so you can add an env for the students' executor so that it doesn't run the local executor, while other usages can still be fulfilled.
张泊明518370910136 commented 2026-03-13 15:11:24 +08:00
Review
Copy Link

What does it mean by students' executor?

What does it mean by students' executor?
蔡雨翔524370910013 commented 2026-03-14 01:46:52 +08:00
Review
Copy Link

What does it mean by students' executor?

I found that I'm wrong. Many stages seem to use the local executor.

> What does it mean by students' executor? I found that I'm wrong. Many stages seem to use the local executor.
蔡雨翔524370910013 commented 2026-03-14 01:47:06 +08:00
Review
Copy Link

This part is not needed.

This part is not needed.
stage.RegisterExecutor(name, &Local{})
}
}