Fix certificate file permissions by devilmonastery · Pull Request #1116 · tobychui/zoraxy

devilmonastery · 2026-04-12T20:18:44Z

add separate file modes for private key files and public certificate or metadata files
enforce secure file modes when ACME renewals overwrite existing cert, key, and json files
apply private and public file permissions to uploaded, bundled localhost, and self-signed certificate files

tobychui · 2026-04-14T10:22:47Z

@devilmonastery This cool!
Let me know when your pull request is ready for review and merged 👍🏻

Copilot

Pull request overview

Addresses insecure certificate/private key file permissions by introducing separate public/private file modes and applying them consistently across TLS cert generation, uploads, and ACME renewals.

Changes:

Added explicit public (0644) vs private key (0600) file modes and a helper to enforce them after writes.
Updated TLS cert upload + self-signed generation paths to write with the appropriate modes.
Updated ACME certificate issuance/renewal writes to enforce modes, and added CLI flags to configure ACME key/public file modes.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/mod/tlscert/tlscert.go	Adds default file modes + helper writer; applies modes to bundled localhost cert/key creation.
src/mod/tlscert/handler.go	Applies mode-aware writes for uploaded cert/key files.
src/mod/tlscert/certgen.go	Applies mode-aware writes for generated self-signed cert/key files.
src/mod/acme/acme.go	Adds configurable file modes to ACME handler and enforces modes on PEM/KEY/JSON outputs.
src/def.go	Adds CLI flags for ACME key/public file modes.
src/acme.go	Parses new CLI flags and passes file modes into ACME handler.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-14T10:53:52Z

src/mod/tlscert/tlscert.go

 	if !utils.FileExists(pubKey) {
 		buildInPubKey, _ := buildinCertStore.ReadFile(filepath.Base(pubKey))
-		os.WriteFile(pubKey, buildInPubKey, 0775)
+		writeFileWithMode(pubKey, buildInPubKey, defaultPublicCertFileMode)
 	}

 	if !utils.FileExists(priKey) {
 		buildInPriKey, _ := buildinCertStore.ReadFile(filepath.Base(priKey))
-		os.WriteFile(priKey, buildInPriKey, 0775)
+		writeFileWithMode(priKey, buildInPriKey, defaultPrivateKeyFileMode)
 	}


NewManager ignores errors from embedded ReadFile and from writeFileWithMode when creating the bundled localhost cert/key. If these writes fail (e.g., missing ./tmp dir, permission issues, or embed read failure), startup continues and later TLS load failures will be harder to diagnose, and key/cert permissions may not be enforced. Please handle and propagate/log these errors (and consider checking os.MkdirAll’s error as well) so initialization fails fast with a clear message.

Copilot · 2026-04-14T10:53:53Z

src/mod/tlscert/handler.go

+	fileMode := defaultPublicCertFileMode
+	if keytype == "pri" {
+		fileMode = defaultPrivateKeyFileMode
 	}
-	defer f.Close()

-	// copy file contents to destination file
-	_, err = io.Copy(f, file)
-	if err != nil {
-		http.Error(w, "Failed to save file", http.StatusInternalServerError)
-		return
-	}
-	_, err = f.Write(fileBytes)
+	err = writeFileWithMode(filepath.Join(m.CertStore, overWriteFilename), fileBytes, fileMode)


The upload filename is derived directly from the user-controlled domain parameter and then passed into filepath.Join(m.CertStore, overWriteFilename). Because domain is not sanitized, values containing path separators (e.g. "../") can escape m.CertStore and overwrite arbitrary files on disk. Please sanitize/validate domain (e.g., strict allowlist for hostnames) and/or enforce that the final cleaned path stays within m.CertStore before writing.

Copilot · 2026-04-14T10:53:53Z

src/mod/acme/acme.go

+	err = a.writeFileWithMode("./conf/certs/"+certificateName+".pem", certificates.Certificate, a.PublicFileMode)
 	if err != nil {
 		a.Logf("Failed to write public key to disk", err)
 		return false, err
 	}
-	err = os.WriteFile("./conf/certs/"+certificateName+".key", certificates.PrivateKey, 0777)
+	err = a.writeFileWithMode("./conf/certs/"+certificateName+".key", certificates.PrivateKey, a.KeyFileMode)
 	if err != nil {


certificateName is ultimately user-controlled (via the filename POST parameter) and is concatenated into paths like ./conf/certs/ + certificateName + ".pem/.key". Without sanitization, certificateName containing path separators (e.g. "../") can escape the cert directory and overwrite arbitrary files. Please validate/normalize the name (e.g. filepath.Base + allowlist) and/or verify the resulting path remains under ./conf/certs before writing.

Copilot · 2026-04-14T10:53:53Z

src/mod/acme/acme.go

 	}

-	err = os.WriteFile("./conf/certs/"+certificateName+".json", certInfoBytes, 0777)
+	err = a.writeFileWithMode("./conf/certs/"+certificateName+".json", certInfoBytes, a.PublicFileMode)


Same path traversal issue as the PEM/KEY writes: certificateName is concatenated into the ./conf/certs/ path for the JSON metadata file without sanitization, allowing "../" to escape the directory and overwrite arbitrary files. Please apply the same filename validation / path containment check here.

Copilot · 2026-04-14T10:53:54Z

src/acme.go

+	keyFileMode := parseACMEFileMode(*acmeKeyFileMode, 0600, "acmekeymode")
+	publicFileMode := parseACMEFileMode(*acmePublicFileMode, 0644, "acmepublicmode")
+
+	return acme.NewACME(strconv.Itoa(port), sysdb, SystemWideLogger, *acmeTestMode, keyFileMode, publicFileMode)
+}
+
+func parseACMEFileMode(raw string, fallback os.FileMode, flagName string) os.FileMode {
+	parsed, err := strconv.ParseUint(raw, 8, 32)
+	if err != nil {
+		SystemWideLogger.Println("Invalid " + flagName + " value " + raw + ", using fallback " + fmt.Sprintf("%#o", fallback))
+		return fallback
+	}
+
+	return os.FileMode(parsed)


parseACMEFileMode accepts any octal value and applies it to the private key files. This allows misconfiguration that makes private keys group/world readable (reintroducing the original security issue) or sets nonsensical mode bits. Consider validating the parsed mode (e.g., for key files require mode&0077 == 0 and possibly mask to permission bits) and fall back to 0600 with a warning if it’s too permissive.

Suggested change

keyFileMode := parseACMEFileMode(*acmeKeyFileMode, 0600, "acmekeymode")

publicFileMode := parseACMEFileMode(*acmePublicFileMode, 0644, "acmepublicmode")

return acme.NewACME(strconv.Itoa(port), sysdb, SystemWideLogger, *acmeTestMode, keyFileMode, publicFileMode)

}

func parseACMEFileMode(raw string, fallback os.FileMode, flagName string) os.FileMode {

parsed, err := strconv.ParseUint(raw, 8, 32)

if err != nil {

SystemWideLogger.Println("Invalid " + flagName + " value " + raw + ", using fallback " + fmt.Sprintf("%#o", fallback))

return fallback

}

return os.FileMode(parsed)

keyFileMode := parseACMEFileMode(*acmeKeyFileMode, 0600, "acmekeymode", true)

publicFileMode := parseACMEFileMode(*acmePublicFileMode, 0644, "acmepublicmode", false)

return acme.NewACME(strconv.Itoa(port), sysdb, SystemWideLogger, *acmeTestMode, keyFileMode, publicFileMode)

}

func parseACMEFileMode(raw string, fallback os.FileMode, flagName string, privateKey bool) os.FileMode {

parsed, err := strconv.ParseUint(raw, 8, 32)

if err != nil {

SystemWideLogger.Println("Invalid " + flagName + " value " + raw + ", using fallback " + fmt.Sprintf("%#o", fallback))

return fallback

}

if parsed&^uint64(os.ModePerm) != 0 {

SystemWideLogger.Println("Invalid " + flagName + " value " + raw + ": contains non-permission bits, using permission bits only")

}

mode := os.FileMode(parsed) & os.ModePerm

if privateKey && mode&0077 != 0 {

SystemWideLogger.Println("Invalid " + flagName + " value " + raw + ": private key files must not be group/world accessible, using fallback " + fmt.Sprintf("%#o", fallback))

return fallback

}

return mode

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-14T10:54:11Z

src/mod/acme/acme.go

+	err = a.writeFileWithMode("./conf/certs/"+certificateName+".key", certificates.PrivateKey, a.KeyFileMode)
 	if err != nil {
 		a.Logf("Failed to write private key to disk", err)
 		return false, err


Same path construction issue for the private key write: certificateName is not validated before being interpolated into the output filename, allowing path traversal / arbitrary overwrite. Sanitizing certificateName once (before the three writes) would address all of these writes consistently.

Copilot · 2026-04-14T10:54:11Z

src/acme.go

+	keyFileMode := parseACMEFileMode(*acmeKeyFileMode, 0600, "acmekeymode")
+	publicFileMode := parseACMEFileMode(*acmePublicFileMode, 0644, "acmepublicmode")
+
+	return acme.NewACME(strconv.Itoa(port), sysdb, SystemWideLogger, *acmeTestMode, keyFileMode, publicFileMode)


The new acmekeymode flag allows configuring a private-key file mode that is insecure (e.g. 0644), which can reintroduce the original vulnerability. Consider enforcing a minimum restriction for keyFileMode (e.g. clear group/other bits or fallback to 0600 with a warning) after parsing, while still allowing acmepublicmode to remain configurable.

Copilot · 2026-04-14T10:54:12Z

src/mod/tlscert/tlscert.go

 	if !utils.FileExists(pubKey) {
 		buildInPubKey, _ := buildinCertStore.ReadFile(filepath.Base(pubKey))
-		os.WriteFile(pubKey, buildInPubKey, 0775)
+		writeFileWithMode(pubKey, buildInPubKey, defaultPublicCertFileMode)
 	}


The embedded localhost cert/key writes ignore both the ReadFile error and the writeFileWithMode error. If the embedded file is missing/corrupt or the write/chmod fails, startup will silently proceed and later TLS fallback may break in non-obvious ways. Please handle the ReadFile/writeFileWithMode errors here (log and/or return the error from NewManager).

Copilot · 2026-04-14T10:54:12Z

src/mod/tlscert/tlscert.go

 	if !utils.FileExists(priKey) {
 		buildInPriKey, _ := buildinCertStore.ReadFile(filepath.Base(priKey))
-		os.WriteFile(priKey, buildInPriKey, 0775)
+		writeFileWithMode(priKey, buildInPriKey, defaultPrivateKeyFileMode)
 	}


The embedded localhost private key write also ignores ReadFile/writeFileWithMode errors. Since this is the default/fallback TLS key, failures should be surfaced (log and/or return an error) rather than continuing with a potentially missing/incorrect key file.

Copilot · 2026-04-14T10:54:12Z

src/mod/acme/acme.go

+	err = a.writeFileWithMode("./conf/certs/"+certificateName+".pem", certificates.Certificate, a.PublicFileMode)
 	if err != nil {
 		a.Logf("Failed to write public key to disk", err)
 		return false, err


certificateName is used directly to construct paths under ./conf/certs/. Since it ultimately comes from an HTTP parameter (filename) and is only stripped of *, an attacker can include path separators (e.g. "../") and write/chmod arbitrary files on the host. Please sanitize/validate certificateName (e.g. filepath.Base + reject any remaining separators / enforce a strict filename pattern) before using it in any filesystem paths.

Fix certificate file permissions

9830461

devilmonastery marked this pull request as ready for review April 14, 2026 10:33

devilmonastery requested a review from tobychui as a code owner April 14, 2026 10:33

tobychui requested review from Copilot April 14, 2026 10:48

Copilot started reviewing on behalf of tobychui April 14, 2026 10:49 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

Conversation

devilmonastery commented Apr 12, 2026

Uh oh!

tobychui commented Apr 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants