[SECURITY] SQL Injection Vulnerability - Use Parameterized Queries

[1234-ad]

🐛 Bug: SQL Injection Vulnerability in User Access Queries

Severity: CRITICAL 🚨

Description

The registration endpoint uses string formatting to construct SQL queries, which is vulnerable to SQL injection attacks. While the current implementation may seem safe because userId comes from the database, this is still a dangerous pattern that should be fixed.

Affected Code

backend/main.go - Lines 207-218:

// ❌ VULNERABLE - String formatting for SQL queries
queryGenAccess := fmt.Sprintf(`INSERT INTO user_access VALUES("%s", "kgp-%%", 1, 0, "")`, userId)
if _, err = db.Exec(queryGenAccess); err != nil {
    // ...
}

queryGenAccess = fmt.Sprintf(`INSERT INTO user_access VALUES("%s", "st_%%", 1, 0, "")`, userId)
if _, err = db.Exec(queryGenAccess); err != nil {
    // ...
}

Why This Is Dangerous

1. SQL Injection Risk: Even though userId comes from the database, using string formatting is a dangerous pattern
2. Code Smell: Sets a bad precedent for other developers
3. Future Vulnerability: If the code is modified to use user input, it becomes exploitable
4. Best Practice Violation: Go's database/sql package provides parameterized queries for a reason

Potential Attack Vector

If the code is ever modified to use user-controlled input:

// If someone changes this in the future:
userId := req.URL.Query().Get("userId") // User input!
queryGenAccess := fmt.Sprintf(`INSERT INTO user_access VALUES("%s", "kgp-%%", 1, 0, "")`, userId)
// Now vulnerable to: userId = `", ""); DROP TABLE user_access; --`

Recommended Solution

Use parameterized queries with placeholders:

// ✅ SECURE - Parameterized queries
queryGenAccess := `INSERT INTO user_access VALUES(?, "kgp-%", 1, 0, "")`
if _, err = db.Exec(queryGenAccess, userId); err != nil {
    fmt.Println("[ERROR] ~ Granting Access to (kgp-*) ~", username, ": ", err.Error())
    http.Error(res, "Internal Server Error", http.StatusInternalServerError)
    return
}

queryGenAccess = `INSERT INTO user_access VALUES(?, "st_%", 1, 0, "")`
if _, err = db.Exec(queryGenAccess, userId); err != nil {
    fmt.Println("[ERROR] ~ Granting Access to (st_*) ~", username, ": ", err.Error())
    http.Error(res, "Internal Server Error", http.StatusInternalServerError)
    return
}

Additional Issues in the Same Code

1. Duplicate Variable Name

// Bad practice - reusing variable name
queryGenAccess := fmt.Sprintf(...)  // First query
queryGenAccess = fmt.Sprintf(...)   // Second query - reusing same variable

Better approach:

// Use descriptive names
kgpAccessQuery := `INSERT INTO user_access VALUES(?, "kgp-%", 1, 0, "")`
stAccessQuery := `INSERT INTO user_access VALUES(?, "st_%", 1, 0, "")`

2. Inconsistent Wildcard Syntax

"kgp-%%"  // Double percent (SQL LIKE pattern)
"st_%%"   // Double percent (SQL LIKE pattern)

Should be:

"kgp-%"   // Single percent (Go string, becomes % in SQL)
"st_%"    // Single percent (Go string, becomes _ in SQL)

Complete Fixed Version

// Provide read-only access for kgp-* channels to the user
kgpAccessQuery := `INSERT INTO user_access (user_id, topic, read, write, owner) VALUES(?, ?, 1, 0, "")`
if _, err = db.Exec(kgpAccessQuery, userId, "kgp-%"); err != nil {
    fmt.Println("[ERROR] ~ Granting Access to (kgp-*) ~", username, ": ", err.Error())
    http.Error(res, "Internal Server Error", http.StatusInternalServerError)
    return
}

// Provide read-only access for st_* channels to the user
stAccessQuery := `INSERT INTO user_access (user_id, topic, read, write, owner) VALUES(?, ?, 1, 0, "")`
if _, err = db.Exec(stAccessQuery, userId, "st_%"); err != nil {
    fmt.Println("[ERROR] ~ Granting Access to (st_*) ~", username, ": ", err.Error())
    http.Error(res, "Internal Server Error", http.StatusInternalServerError)
    return
}

Benefits of Parameterized Queries

Aspect	String Formatting	Parameterized Queries
SQL Injection	❌ Vulnerable	✅ Protected
Type Safety	❌ No validation	✅ Type checked
Performance	⚠️ No caching	✅ Query plan cached
Readability	⚠️ Harder to read	✅ Clearer intent
Maintainability	❌ Error-prone	✅ Safer to modify

Testing

// Test with malicious input (should be safely escaped)
userId := `1"); DROP TABLE user_access; --`
query := `INSERT INTO user_access VALUES(?, "kgp-%", 1, 0, "")`
_, err := db.Exec(query, userId)
// With parameterized queries, this is safely escaped
// Without them, this would execute the DROP TABLE command

References

• OWASP SQL Injection
• Go database/sql Best Practices
• CWE-89: SQL Injection

Priority

CRITICAL - Should be fixed immediately to follow security best practices.

---

Note: While the current code may not be directly exploitable, using parameterized queries is a fundamental security practice that should always be followed.