R-1126: the gate publishes the container it checked, by id (TOCTOU from the commit's security review)
gates / gates (push) Successful in 42s
gates / gates (push) Successful in 42s
web.SyncFileBrowserMounts can re-create FileBrowser between the check and the connect; a connect by name would publish the new, unchecked container. The run is now "<id> <StartedAt>" and the connect targets that id. TestFileBrowserGate_ConnectsTheCheckedContainerById, red-proved. Claude-Session: https://claude.ai/code/session_0159rPz1ZhFKsS53msqPYxtS
This commit is contained in:
1 parent
411d7761d9
commit
8f029aa70b
3 files changed
+37
-5
No files matched your search
@@ -70,15 +70,24 @@ func (m *Manager) joinFileBrowserNetwork() {
|
|||||||
|
|
||||||
// ensureFileBrowserPublic joins FileBrowser to traefik-public only when THIS run was checked and its default login is
|
// ensureFileBrowserPublic joins FileBrowser to traefik-public only when THIS run was checked and its default login is
|
||||||
// refused. Anything less — the run unknown, not checked yet, the check failed — leaves it unreachable from outside.
|
// refused. Anything less — the run unknown, not checked yet, the check failed — leaves it unreachable from outside.
|
||||||
|
//
|
||||||
|
// TOCTOU (found by the commit's security review, 2026-10-17): the connect targets the CONTAINER ID read with the run,
|
||||||
|
// never the name — web.SyncFileBrowserMounts can re-create FileBrowser between the check and the connect, and a
|
||||||
|
// connect by name would publish the new, unchecked container. By id, a replaced container's connect simply fails.
|
||||||
|
// Pinned by TestFileBrowserGate_ConnectsTheCheckedContainerById.
|
||||||
func (m *Manager) ensureFileBrowserPublic() {
|
func (m *Manager) ensureFileBrowserPublic() {
|
||||||
run := m.fbStartedAt()
|
run := m.fbStartedAt()
|
||||||
if run == "" || run != m.fbProbedRun {
|
if run == "" || run != m.fbProbedRun {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
if m.fbNetOn(fileBrowserContainer, traefikNetwork) {
|
target := fileBrowserContainer // tests' runs carry no id
|
||||||
|
if f := strings.Fields(run); len(f) == 2 {
|
||||||
|
target = f[0]
|
||||||
|
}
|
||||||
|
if m.fbNetOn(target, traefikNetwork) {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
if err := m.fbNetConnect(fileBrowserContainer, traefikNetwork); err != nil {
|
if err := m.fbNetConnect(target, traefikNetwork); err != nil {
|
||||||
m.logger.Printf("[WARN] [infra] filebrowser: could not join %s (files.<domain> stays unreachable): %v", traefikNetwork, err)
|
m.logger.Printf("[WARN] [infra] filebrowser: could not join %s (files.<domain> stays unreachable): %v", traefikNetwork, err)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -76,8 +76,10 @@ func fbLogin(do fbHTTPDo, base, password string) (string, int, error) {
|
|||||||
return strings.Trim(strings.TrimSpace(string(b)), `"`), resp.StatusCode, nil
|
return strings.Trim(strings.TrimSpace(string(b)), `"`), resp.StatusCode, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// fbStartedAt identifies FileBrowser's current run (its container's .State.StartedAt); "" when unknown. Tests set
|
// fbStartedAt identifies FileBrowser's current run: "<container id> <.State.StartedAt>" (R-1126: the gate connects that
|
||||||
// fbStarted; with a test FileBrowser (fbHTTP / fbBaseURL) and no fbStarted it is "" — a test never reaches docker.
|
// exact container id, so a container re-created between the check and the connect is never published). "" when
|
||||||
|
// unknown. Tests set fbStarted; with a test FileBrowser (fbHTTP / fbBaseURL) and no fbStarted it is "" — a test never
|
||||||
|
// reaches docker.
|
||||||
func (m *Manager) fbStartedAt() string {
|
func (m *Manager) fbStartedAt() string {
|
||||||
if m.fbStarted != nil {
|
if m.fbStarted != nil {
|
||||||
return m.fbStarted()
|
return m.fbStarted()
|
||||||
@@ -85,7 +87,7 @@ func (m *Manager) fbStartedAt() string {
|
|||||||
if m.fbHTTP != nil || m.fbBaseURL != "" {
|
if m.fbHTTP != nil || m.fbBaseURL != "" {
|
||||||
return ""
|
return ""
|
||||||
}
|
}
|
||||||
out, err := dockerexec.Command("docker", "inspect", "--format", "{{.State.StartedAt}}", fileBrowserContainer).Output()
|
out, err := dockerexec.Command("docker", "inspect", "--format", "{{.Id}} {{.State.StartedAt}}", fileBrowserContainer).Output()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return ""
|
return ""
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -296,3 +296,24 @@ func TestFileBrowserGate_PublishedOnlyAfterTheCheck(t *testing.T) {
|
|||||||
t.Fatalf("a checked restart did more: %v", events)
|
t.Fatalf("a checked restart did more: %v", events)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// R-1126 TOCTOU: the gate connects the container whose run it checked, by ID — so a FileBrowser re-created between the
|
||||||
|
// check and the connect (web.SyncFileBrowserMounts runs outside infraMu) is never the one published.
|
||||||
|
// RED-PROOF (2026-10-17): connecting by the name „filebrowser" fails this test.
|
||||||
|
func TestFileBrowserGate_ConnectsTheCheckedContainerById(t *testing.T) {
|
||||||
|
fb := &fakeFB{password: "admin"}
|
||||||
|
srv := httptest.NewServer(fb.handler())
|
||||||
|
defer srv.Close()
|
||||||
|
m, _, _ := fbManager(t, srv.URL)
|
||||||
|
m.fbStarted = func() string { return "3f2a9c 2026-10-17T11:20:00Z" }
|
||||||
|
var connected []string
|
||||||
|
m.fbNetOnFn = func(c, n string) bool { return false }
|
||||||
|
m.fbNetConnectFn = func(c, n string) error {
|
||||||
|
connected = append(connected, c+"@"+n)
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
m.fileBrowserGateLocked()
|
||||||
|
if len(connected) != 2 || connected[1] != "3f2a9c@traefik-public" {
|
||||||
|
t.Fatalf("the checked container must be published by its id, got %v", connected)
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in new issue
Block a user