Nginx UI OTP secure-session step-up is missing on sensitive mutation routes

Reporter: Jagadesh Achanta Target: `0xJacky/nginx-ui` Affected version / commit tested: - App version: `2.3.10` - Commit: `4948fba8744e82c3d7003f68ee164e9f49421066` - Disclosure channel: GitHub Private Vulnerability Reporting - Credential reference: `cred:github-jagstack-gh-cli` ## AI assistance disclosure This report was prepared by Jagadesh Achanta with assistance from OpenAI/Codex for public source review, local evidence collection, and report drafting. ## Summary Nginx UI has a short-lived OTP/passkey "secure session" step-up boundary for users with 2FA enabled. `RequireSecureSession()` rejects an OTP-enabled user's request unless the request includes a valid `X-Secure-Session-ID` issued after a fresh OTP/passkey verification. The project already applies this step-up middleware to sensitive operations such as: - Nginx config create/edit/rename/delete; - site save/delete/enable/disable/rename/duplicate; - stream save/delete/enable/disable/rename/duplicate; - settings save and protected secret reads; - backup restore. However, several similarly sensitive authenticated mutation routes remain outside `RequireSecureSession()`. An attacker who obtains an authenticated JWT/session for a 2FA-enabled user, but does not have the victim's fresh OTP/passkey secure-session token, can still perform those operations. The affected route groups include: - automatic backup create/modify/delete/restore/test-S3 operations; - DNS credential create/edit/delete operations; - certificate and ACME user create/modify/delete/register operations; - DNS domain, record, and DDNS create/update/delete operations; - Nginx reload/restart, stub-status toggle, performance-setting update, and module-cache refresh operations; - site/stream advanced-mode toggles and batch updates; - auto-certificate binding/unbinding for sites; - site navigation order and health-check updates. This report is distinct from my earlier Nginx UI report about auto-backup path traversal. This report is about missing 2FA step-up authorization on sensitive authenticated mutation routes. ## Why this is a security boundary `internal/middleware/secure_session.go` only requires a secure-session token for users with OTP enabled. For OTP users, it reads `X-Secure-Session-ID` from the header or query string and aborts with `401 Unauthorized` if the secure session is missing or invalid: ```go if !cUser.EnabledOTP() { c.Next() return } ssid := c.GetHeader("X-Secure-Session-ID") if ssid == "" { ssid = c.Query("X-Secure-Session-ID") } if ssid == "" { c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{ "message": "Secure Session ID is empty", }) return } if user.VerifySecureSessionID(ssid, cUser.ID) { c.Next() return } ``` `internal/user/otp.go` issues the secure-session ID for only 10 minutes: ```go func SetSecureSessionID(userId uint64) (sessionId string) { sessionId = uuid.NewString() cache.Set(secureSessionIDCacheKey(sessionId), userId, 10*time.Minute) return } ``` The UI/server already rely on this boundary for sensitive actions. For example, `api/config/router.go` wraps config mutations with `RequireSecureSession()`: ```go o := r.Group("", middleware.RequireSecureSession()) { o.POST("configs", AddConfig) o.POST("config", EditConfig) o.POST("config_mkdir", Mkdir) o.POST("config_rename", Rename) o.POST("config_delete", DeleteConfig) } ``` `api/sites/router.go` protects normal site mutations: ```go o := r.Group("", middleware.RequireSecureSession()) { o.POST("sites/:name/rename", RenameSite) o.POST("sites/:name/enable", EnableSite) o.POST("sites/:name/disable", DisableSite) o.POST("sites/:name", SaveSite) o.DELETE("sites/:name", DeleteSite) o.POST("sites/:name/duplicate", DuplicateSite) o.POST("sites/:name/maintenance", EnableMaintenanceSite) } ``` `api/streams/router.go` protects normal stream mutations, and `api/settings/router.go` protects secret reads/settings saves: ```go o := r.Group("", middleware.RequireSecureSession()) { o.POST("streams/:name", SaveStream) o.POST("streams/:name/rename", RenameStream) o.POST("streams/:name/enable", EnableStream) o.POST("streams/:name/disable", DisableStream) o.DELETE("streams/:name", DeleteStream) o.POST("streams/:name/duplicate", Duplicate) } ``` ```go r.GET("settings/protected", middleware.RequireSecureSession(), GetProtectedSetting) r.POST("settings", middleware.RequireSecureSession(), SaveSettings) ``` The repository's own tests also verify this expected behavior. `api/config/security_test.go`, `api/sites/security_test.go`, and `api/streams/security_test.go` each assert that OTP-enabled users receive `401 Unauthorized` when protected mutations are attempted without a secure session. ## Affected routes outside the step-up boundary The affected routes are under the authenticated private router: ```go g := root.Group("/", middleware.AuthRequired(), middleware.Proxy()) ``` but they do not add `middleware.RequireSecureSession()`. ### Automatic backup management `api/backup/router.go` protects manual restore: ```go r.POST("/restore", middleware.AuthRequired(), middleware.RequireSecureSession(), middleware.EncryptedForm(), RestoreBackup) ``` but the automatic backup management router has no secure-session middleware: ```go r.POST("/auto_backup", CreateAutoBackup) r.POST("/auto_backup/:id", ModifyAutoBackup) r.DELETE("/auto_backup/:id", DestroyAutoBackup) r.PATCH("/auto_backup/:id", RestoreAutoBackup) r.POST("/auto_backup/test_s3", TestS3Connection) ``` Those handlers create or modify backup jobs, register cron jobs, test attacker-supplied S3 settings, and store S3 credentials: ```go "s3_access_key_id": "omitempty", "s3_secret_access_key": "omitempty", ... if ctx.Model.Enabled { if err := cron.AddAutoBackupJob(ctx.Model.ID, ctx.Model.CronExpression); err != nil { ``` The model stores S3 secret material: ```go S3AccessKeyID string `json:"s3_access_key_id" gorm:"comment:S3 access key ID;serializer:json[aes]"` S3SecretAccessKey string `json:"s3_secret_access_key" gorm:"comment:S3 secret access key;serializer:json[aes]"` ``` ### Certificate, ACME, and DNS credential management `api/certificate/router.go` registers credential and certificate mutations without `RequireSecureSession()`: ```go r.POST("dns_credentials", AddDnsCredential) r.POST("dns_credentials/:id", EditDnsCredential) r.DELETE("dns_credentials/:id", DeleteDnsCredential) r.POST("certs", AddCert) r.POST("certs/:id", ModifyCert) r.DELETE("certs/:id", RemoveCert) r.PUT("cert_sync", SyncCertificate) r.POST("acme_users", CreateAcmeUser) r.POST("acme_users/:id", ModifyAcmeUser) r.POST("acme_users/:id/register", RegisterAcmeUser) r.DELETE("acme_users/:id", DestroyAcmeUser) ``` The DNS credential handlers persist provider configuration: ```go dnsCredential := model.DnsCredential{ Name: json.Name, Config: &json.Config, Provider: json.Provider, ProviderCode: providerCode, } ... err := d.Create(&dnsCredential) ``` The model serializes that configuration with AES: ```go Config *dns.Config `json:"config,omitempty" gorm:"serializer:json[aes]"` ``` ### DNS domains, records, and DDNS `api/dns/router.go` registers DNS domain, record, and DDNS mutations without secure-session middleware: ```go group.POST("/domains", CreateDomain) group.POST("/domains/:id", UpdateDomain) group.DELETE("/domains/:id", DeleteDomain) group.POST("/domains/:id/records", CreateRecord) group.PUT("/domains/:id/records/:record_id", UpdateRecord) group.DELETE("/domains/:id/records/:record_id", DeleteRecord) group.PUT("/domains/:id/ddns", UpdateDDNSConfig) group.DELETE("/domains/:id/ddns", DeleteDDNSConfig) ``` Those handlers call the DNS provider service and modify DDNS jobs: ```go record, err := svc.CreateRecord(c.Request.Context(), domainID, toRecordInput(payload)) ... record, err := svc.UpdateRecord(c.Request.Context(), domainID, recordID, toR