[Opus 4.6 PR-1] Insecure Randomness in Token/Secret Generation (base.go)

base/base.go

@@ -3,24 +3,24 @@ package base
 import (
 	"bytes"
 	"crypto/hmac"
+	"crypto/rand"
 	"crypto/sha1"
 	"crypto/tls"
 	"encoding/base64"
 	"fmt"
+	"github.com/spf13/viper"
+	"golang.org/x/crypto/bcrypt"
 	"io/ioutil"
-	"math/rand"
+	"math/big"
 	"net"
 	"net/http"
 	"net/mail"
 	"net/smtp"
 	"strings"
 	"time"
-
-	"github.com/spf13/viper"
-	"golang.org/x/crypto/bcrypt"
 )
 
-// User structure holds authorized users details
+//User structure holds authorized users details
 type User struct {
 	Nickname         string
 	Password         string
@@ -38,50 +38,51 @@ type User struct {
 
 var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789+/")
 var simpleLetters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789")
-var randInit = 0
 
-// MaxAge defines cookie expiration
+//MaxAge defines cookie expiration
 var MaxAge = 3600 * 24
 
 func randAlphaSlashPlus(n int) string {
-	if randInit == 0 {
-		rand.Seed(time.Now().UnixNano())
-	}
 	b := make([]rune, n)
 	for i := range b {
-		b[i] = letters[rand.Intn(len(letters))]
+		idx, err := rand.Int(rand.Reader, big.NewInt(int64(len(letters))))
+		if err != nil {
+			panic("crypto/rand failed: " + err.Error())
+		}
+		b[i] = letters[idx.Int64()]
 	}
 	return string(b)
 }
 
 func randAlpha(n int) string {
-	if randInit == 0 {
-		rand.Seed(time.Now().UnixNano())
-	}
 	b := make([]rune, n)
 	for i := range b {
-		b[i] = simpleLetters[rand.Intn(len(simpleLetters))]
+		idx, err := rand.Int(rand.Reader, big.NewInt(int64(len(simpleLetters))))
+		if err != nil {
+			panic("crypto/rand failed: " + err.Error())
+		}
+		b[i] = simpleLetters[idx.Int64()]
 	}
 	return string(b)
 }
 
-// GenerateAccountACKLink generates account verification link
+//GenerateAccountACKLink generates account verification link
 func GenerateAccountACKLink(length int) string {
 	return randAlpha(length)
 }
 
-// GenerateAuthToken creates auth token for created user
+//GenerateAuthToken creates auth token for created user
 func GenerateAuthToken(TokenType string, length int) string {
 	return randAlphaSlashPlus(length)
 }
 
-// HashPassword gets hash from password
+//HashPassword gets hash from password
 func HashPassword(password string) (string, error) {
 	bytes, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost)
 	return string(bytes), err
 }
 
-// CheckPasswordHash checks given password
+//CheckPasswordHash checks given password
 func CheckPasswordHash(password, hash string) bool {
 	err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password))
 	return err == nil
@@ -112,7 +113,7 @@ func initSmtpconfig() error {
 	return nil
 }
 
-// SendEmail provides email function for varied interactions
+//SendEmail provides email function for varied interactions
 func SendEmail(email string, subject string, validationString string) {
 	var auth smtp.Auth
 	err := initSmtpconfig()
@@ -268,7 +269,7 @@ func SendEmail(email string, subject string, validationString string) {
 
 }
 
-// Request handler
+//Request handler
 func Request(method string, resURI string, Path string, Data string, content []byte, query string, Key string, SecretKey string) (*http.Response, error) {
 
 	client := &http.Client{}

tests/TESTS.md

@@ -0,0 +1,37 @@
+# OSFCI Test Suite
+
+This document tracks all test files created as part of the security hardening effort.
+Each section corresponds to a PR and lists every test with what it validates.
+
+---
+
+## PR 1 — Crypto-secure Token Generation
+
+**Files:** `tests/base/base.go`, `tests/base/base_test.go`
+
+**What changed:** Replaced `math/rand` (predictable PRNG) with `crypto/rand` (OS CSPRNG) for all token, secret, and validation link generation. Removed the racy `randInit` global variable.
+
+| Test | What it validates |
+|------|-------------------|
+| `TestRandAlphaSlashPlusLength` | Correct output length for various sizes |
+| `TestRandAlphaLength` | Correct output length for various sizes |
+| `TestRandAlphaSlashPlusCharset` | Only valid characters produced (1000-char sample) |
+| `TestRandAlphaCharset` | Only alphanumeric chars, no `+/` leakage |
+| `TestRandAlphaSlashPlusUniqueness` | No collisions across 10k 32-byte tokens |
+| `TestRandAlphaUniqueness` | No collisions across 10k 32-byte tokens |
+| `TestGenerateAuthTokenLength` | Public API returns correct length (40) |
+| `TestGenerateAccountACKLinkLength` | Public API returns correct length (24) |
+| `TestGenerateAccountACKLinkNoSpecialChars` | ACK links use safe alphabet only |
+| `TestRandAlphaConcurrentSafety` | 10 goroutines calling simultaneously — run with `go test -race` |
+
+**Setup example**
+
+```cd ~/code/osfci
+go mod init github.com/arunkoshy/OSF-OSFCI
+go mod tidy
+```
+**Run:**
+
+```bash
+cd tests/base && go test -v -race .
+```

tests/base/base.go

@@ -0,0 +1,49 @@
+// Extracted from base/base.go for testing crypto/rand token generation.
+// This file mirrors the production functions so tests can validate them
+// without requiring the full module dependency graph (viper, smtp, etc).
+
+package main
+
+import (
+	"crypto/rand"
+	"math/big"
+)
+
+var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789+/")
+var simpleLetters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789")
+
+func randAlphaSlashPlus(n int) string {
+	b := make([]rune, n)
+	for i := range b {
+		idx, err := rand.Int(rand.Reader, big.NewInt(int64(len(letters))))
+		if err != nil {
+			panic("crypto/rand failed: " + err.Error())
+		}
+		b[i] = letters[idx.Int64()]
+	}
+	return string(b)
+}
+
+func randAlpha(n int) string {
+	b := make([]rune, n)
+	for i := range b {
+		idx, err := rand.Int(rand.Reader, big.NewInt(int64(len(simpleLetters))))
+		if err != nil {
+			panic("crypto/rand failed: " + err.Error())
+		}
+		b[i] = simpleLetters[idx.Int64()]
+	}
+	return string(b)
+}
+
+// GenerateAccountACKLink generates account verification link
+func GenerateAccountACKLink(length int) string {
+	return randAlpha(length)
+}
+
+// GenerateAuthToken creates auth token for created user
+func GenerateAuthToken(TokenType string, length int) string {
+	return randAlphaSlashPlus(length)
+}
+
+func main() {}

tests/base/base_test.go

@@ -0,0 +1,112 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestRandAlphaSlashPlusLength(t *testing.T) {
+	for _, length := range []int{0, 1, 10, 20, 40, 64} {
+		result := randAlphaSlashPlus(length)
+		if len(result) != length {
+			t.Errorf("randAlphaSlashPlus(%d) returned length %d", length, len(result))
+		}
+	}
+}
+
+func TestRandAlphaLength(t *testing.T) {
+	for _, length := range []int{0, 1, 10, 20, 40, 64} {
+		result := randAlpha(length)
+		if len(result) != length {
+			t.Errorf("randAlpha(%d) returned length %d", length, len(result))
+		}
+	}
+}
+
+func TestRandAlphaSlashPlusCharset(t *testing.T) {
+	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789+/"
+	result := randAlphaSlashPlus(1000)
+	for _, c := range result {
+		if !strings.ContainsRune(charset, c) {
+			t.Errorf("randAlphaSlashPlus produced invalid character: %c", c)
+		}
+	}
+}
+
+func TestRandAlphaCharset(t *testing.T) {
+	const charset = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
+	result := randAlpha(1000)
+	for _, c := range result {
+		if !strings.ContainsRune(charset, c) {
+			t.Errorf("randAlpha produced invalid character: %c", c)
+		}
+	}
+	// Ensure no slash or plus characters appear (those belong to the other alphabet)
+	if strings.ContainsAny(result, "+/") {
+		t.Error("randAlpha should not produce + or / characters")
+	}
+}
+
+func TestRandAlphaSlashPlusUniqueness(t *testing.T) {
+	seen := make(map[string]bool)
+	for i := 0; i < 10000; i++ {
+		token := randAlphaSlashPlus(32)
+		if seen[token] {
+			t.Fatalf("randAlphaSlashPlus produced duplicate token on iteration %d", i)
+		}
+		seen[token] = true
+	}
+}
+
+func TestRandAlphaUniqueness(t *testing.T) {
+	seen := make(map[string]bool)
+	for i := 0; i < 10000; i++ {
+		token := randAlpha(32)
+		if seen[token] {
+			t.Fatalf("randAlpha produced duplicate token on iteration %d", i)
+		}
+		seen[token] = true
+	}
+}
+
+func TestGenerateAuthTokenLength(t *testing.T) {
+	token := GenerateAuthToken("mac", 40)
+	if len(token) != 40 {
+		t.Errorf("GenerateAuthToken returned length %d, want 40", len(token))
+	}
+}
+
+func TestGenerateAccountACKLinkLength(t *testing.T) {
+	link := GenerateAccountACKLink(24)
+	if len(link) != 24 {
+		t.Errorf("GenerateAccountACKLink returned length %d, want 24", len(link))
+	}
+}
+
+func TestGenerateAccountACKLinkNoSpecialChars(t *testing.T) {
+	// ACK links use randAlpha which should not contain + or /
+	for i := 0; i < 100; i++ {
+		link := GenerateAccountACKLink(24)
+		if strings.ContainsAny(link, "+/") {
+			t.Errorf("GenerateAccountACKLink should not contain + or /: got %s", link)
+		}
+	}
+}
+
+func TestRandAlphaConcurrentSafety(t *testing.T) {
+	// Run with -race flag to detect data races:
+	//   go test -race ./tests/base/
+	done := make(chan bool, 10)
+	for i := 0; i < 10; i++ {
+		go func() {
+			for j := 0; j < 100; j++ {
+				_ = randAlpha(32)
+				_ = randAlphaSlashPlus(32)
+			}
+			done <- true
+		}()
+	}
+	for i := 0; i < 10; i++ {
+		<-done
+	}
+}