security: prioritized remediation plan - plaintext passwords, SQL injection, path traversal

[dvrd]

Why

Security audit from rounds 56-60 identified critical vulnerabilities requiring immediate remediation. Prioritized action plan focuses on highest-severity issues first.

What

Implement prioritized security fixes in following order:

1. Fix plaintext password storage vulnerability (CRITICAL)
2. Add SQL injection prevention (CRITICAL)
3. Add path traversal prevention (HIGH)
4. Fix user deletion race condition (HIGH)
5. Add connection pool limits (HIGH)

Where

• pkg/store/database/user.go - User password storage
• pkg/store/database/repo.go - Repository operations
• pkg/backend/user.go - User management
• pkg/db/db.go - Database connection management
• pkg/utils/utils.go - Input sanitization
• pkg/web/auth.go - Authentication flow
• Various input endpoints across pkg/web/ and pkg/ssh/cmd/

Plan

Phase 1: Fix Plaintext Password Storage (CRITICAL) ✅

Status: PR #866 created and open in dvrd/soft-serve

File: pkg/store/database/user.go
Changes:

• Added validateBcryptHash() helper function to check password format (60 chars, starts with $2a$ or $2b$)
• Updated SetUserPassword() to validate password before storage
• Updated SetUserPasswordByUsername() to validate password before storage
• Added unit tests for bcrypt hash validation

Test: Unit tests verify bcrypt hash validation works correctly
Risk: Database backup files could contain plaintext passwords
PR: https://github.com/dvrd/soft-serve/pull/866

Phase 2: SQL Injection Prevention (CRITICAL)

File: pkg/store/database/repo.go
Changes:

1. Find all instances of string concatenation in SQL queries
2. Replace with parameterized queries using tx.Rebind() or tx.RebindNamed():

   // Replace this pattern:
   tx.Rebind("SELECT * FROM repos WHERE name = $1", sql.Named("repo", name))
   
   // With proper handling:
   repo := struct{Name string}
   if err := tx.Get(&repo, sql.Named("repo", $1)); err != nil {
       return err
   }

Test: Attempt SQL injection via malicious repository name
Risk: Full database compromise via SQL injection
PR: security/fix-sql-injection

Phase 3: Path Traversal Prevention (HIGH)

File: pkg/utils/utils.go
Changes:

1. Enhance SanitizeRepo() to prevent ../ sequences:

   func SanitizeRepo(repo string) string {
       repo = Sanitize(repo)
       // Remove leading slashes
       repo = strings.TrimPrefix(repo, "/")
       
       // Prevent path traversal - check for ../
       if strings.Contains(repo, "../") {
           return "", fmt.Errorf("invalid repository name: path sequences not allowed")
       }
       
       // Existing sanitization
       repo = "/" + repo
       repo = path.Clean(repo)
       repo = strings.TrimSuffix(repo, ".git")
       return strings.TrimPrefix(repo, "/")
   }

2. Update call sites in pkg/web/blob.go and pkg/web/git.go to use SanitizeRepo() before using repo parameter

Test: Attempt to access ../../etc/passwd via malicious repo name
Risk: File system access bypass via path traversal
PR: security/fix-path-traversal

Phase 4: Fix User Deletion Race (HIGH)

File: pkg/backend/user.go
Changes:
Move repo deletion inside a database transaction:

return dbx.TransactionContext(ctx, func(tx *db.Tx) error {
    // Fetch repos to delete first
    repos, err := store.GetRepositoriesByUserID(ctx, tx, userID)
    if err != nil {
        return db.WrapError(err)
    }
    
    // Delete repos
    for _, repo := range repos {
        if err := store.DeleteRepository(ctx, tx, repo.ID()); err != nil {
            return db.WrapError(err)
        }
    }
    
    return nil
})

Test: Concurrent user deletion attempts
Risk: Orphaned repositories and database inconsistency
PR: security/fix-user-deletion-race

Phase 5: Connection Pool Limits (HIGH)

File: pkg/db/db.go
Changes:

1. Add connection limit constants:

   const (
       maxOpenConnections = 25
       maxIdleConnections = 10
   )
   
   // In Open() function, configure pool:
   db.SetMaxOpenConns(maxOpenConnections)
   db.SetMaxIdleConns(maxIdleConnections)
   db.SetConnMaxLifetime(30 * time.Minute)

Test: Load test with 25 concurrent connections
Risk: DoS via connection exhaustion under load
PR: security/fix-connection-pool-limits

Testing Strategy

• Unit tests for each fix
• Integration test for concurrent operations
• Manual security review for each PR
• Load testing for connection limits

Order of Implementation

1. Phase 1 → Phase 2 → Phase 3 → Phase 4 → Phase 5
2. Each phase: implement, test, create PR in dvrd/soft-serve
3. Merge each PR into main after tests pass
4. Document test results in PR description

Success Criteria

• All 5 PRs created in dvrd/soft-serve
• All unit tests passing
• Integration tests passing
• No security regressions
• Connection limits functioning correctly

Notes

This is a CRITICAL security remediation project. Each fix should be:

1. Implemented with comprehensive tests
2. Reviewed by security expert (internal or external)
3. Tested in staging environment before production
4. Backed by canary plan (optional rollback strategy)

[dvrd]

Phase 1 PR created: #866

Changes:

• Added validateBcryptHash() helper function
• Updated SetUserPassword() and SetUserPasswordByUsername() to validate passwords
• Added unit tests

This prevents plaintext password storage at the database layer.