Fix trial cleanup, users.db perms, DB field escaping and /tmp races

- create_trial_account wrote a 6-field record, so the marker landed in
  field 6 while firewallfalcon-trial-cleanup.sh reads field 7. Trials were
  never actually removed. Write the daily-bandwidth field so the layout
  matches, and drop a <user>.trial_expiry stamp so the limiter's sweep --
  previously dead code, nothing ever created those files -- can act as a
  fallback when atd is unavailable.
- create_user tagged every normal account as "trial"; use "normal" so the
  now-working cleanup cannot delete a regular user. Also remove the
  trial_expiry stamp when an account is deleted.
- ensure_firewallfalcon_dirs now chmods users.db to 0600; it stores
  cleartext passwords and was created world-readable by touch.
- Replace the `sed -i "s/^user:.*/..."` record updaters with an awk-based
  db_set_user_field. Values containing / or & are now stored literally and
  the trailing marker field is preserved (renew_user also truncated the
  record to five fields).
- Validate operator-supplied passwords: ':' breaks the record layout, a
  backslash is eaten by awk -v, quotes and whitespace break the consumers.
- Use mktemp instead of the fixed /tmp/ff_banners_new.conf and
  /tmp/badvpn_build paths, which root wrote in a world-writable directory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

menu.sh

@@ -403,6 +403,9 @@ check_environment() {
 ensure_firewallfalcon_dirs() {
     mkdir -p "$DB_DIR" "$SSL_CERT_DIR" "$BANDWIDTH_DIR" /etc/ssh/sshd_config.d
     touch "$DB_FILE"
+    # users.db holds cleartext credentials and lives on a box where the managed
+    # users have accounts; keep it readable by root only.
+    chmod 600 "$DB_FILE" 2>/dev/null
 }
 
 ensure_firewallfalcon_system_group() {
@@ -414,6 +417,49 @@ db_has_user() {
     awk -F: -v target="$1" '$1 == target { found=1; exit } END { exit(found ? 0 : 1) }' "$DB_FILE"
 }
 
+# users.db fields: 1=user 2=pass 3=expiry 4=conn_limit 5=bandwidth 6=daily_bandwidth 7=marker
+DB_FIELD_PASS=2
+DB_FIELD_EXPIRY=3
+DB_FIELD_LIMIT=4
+DB_FIELD_BW=5
+DB_FIELD_DAILY_BW=6
+
+# Rewrite a single field of a user's record. Uses awk instead of `sed s/^user:.*/.../`
+# so that values containing / & \ are stored literally, and so the trailing marker
+# field is preserved instead of being dropped.
+db_set_user_field() {
+    local username="$1" field="$2" value="$3" tmp
+    [[ -f "$DB_FILE" ]] || return 1
+    tmp=$(mktemp) || return 1
+    if ! awk -F: -v OFS=: -v u="$username" -v f="$field" -v v="$value" \
+        '$1 == u { $f = v } { print }' "$DB_FILE" > "$tmp"; then
+        rm -f "$tmp"
+        return 1
+    fi
+    # Copy contents rather than mv so the 0600 mode of users.db survives.
+    cat "$tmp" > "$DB_FILE"
+    rm -f "$tmp"
+}
+
+# Reject passwords that would corrupt the ':'-delimited record or the shell
+# pipelines that consume it.
+ff_is_valid_password() {
+    local pw="$1"
+    if [[ -z "$pw" ]]; then
+        echo -e "\n${C_RED}❌ Password cannot be empty.${C_RESET}"
+        return 1
+    fi
+    # Quoted literals rather than backslash-escaped case patterns: the latter are
+    # easy to get subtly wrong. ':' breaks the record layout, a backslash is eaten
+    # by awk -v, and quotes/whitespace break the shell pipelines that consume it.
+    if [[ "$pw" == *:* || "$pw" == *'\'* || "$pw" == *"'"* || "$pw" == *'"'* ]] \
+       || [[ "$pw" =~ [[:space:]] ]]; then
+        echo -e "\n${C_RED}❌ Password cannot contain ':', backslash, quotes or whitespace.${C_RESET}"
+        return 1
+    fi
+    return 0
+}
+
 is_firewallfalcon_orphan_user() {
     local username="$1"
     local passwd_line system_user _ uid _ home shell
@@ -497,6 +543,7 @@ delete_firewallfalcon_user_accounts() {
         rm -f "$BANDWIDTH_DIR/${username}.daily_usage"
         rm -f "$BANDWIDTH_DIR/${username}.conn_locked"
         rm -f "$BANDWIDTH_DIR/${username}.daily_locked"
+        rm -f "$BANDWIDTH_DIR/${username}.trial_expiry"
         rm -rf "$BANDWIDTH_DIR/pidtrack/${username}"
     done
 
@@ -1222,7 +1269,8 @@ update_ssh_banners_config() {
     fi
 
     ensure_firewallfalcon_dirs
-    tmp_conf="/tmp/ff_banners_new.conf"
+    # mktemp, not a fixed /tmp name: root writes this file and /tmp is world-writable.
+    tmp_conf=$(mktemp) || return
     echo "# FirewallFalcon - Dynamic per-user SSH banners" > "$tmp_conf"
 
     if [[ -f "$DB_FILE" ]]; then
@@ -1235,6 +1283,7 @@ update_ssh_banners_config() {
 
     if ! cmp -s "$tmp_conf" "$SSHD_FF_CONFIG" 2>/dev/null; then
         mv "$tmp_conf" "$SSHD_FF_CONFIG"
+        chmod 644 "$SSHD_FF_CONFIG"
         if ! grep -q "^Include /etc/ssh/sshd_config.d/" /etc/ssh/sshd_config 2>/dev/null; then
             echo "Include /etc/ssh/sshd_config.d/*.conf" >> /etc/ssh/sshd_config
         fi
@@ -1628,7 +1677,7 @@ create_user() {
             password=$(tr -dc 'A-Za-z0-9' < /dev/urandom | head -c 8)
             echo -e "${C_GREEN}🔑 Auto-generated password: ${C_YELLOW}$password${C_RESET}"
             break
-        else
+        elif ff_is_valid_password "$password"; then
             break
         fi
     done
@@ -1654,7 +1703,7 @@ create_user() {
     fi
     usermod -aG "$FF_USERS_GROUP" "$username" 2>/dev/null
     echo "$username:$password" | chpasswd; chage -E "$expire_date" "$username"
-    echo "$username:$password:$expire_date:$limit:$bandwidth_gb:$daily_bandwidth_gb:trial" >> "$DB_FILE"
+    echo "$username:$password:$expire_date:$limit:$bandwidth_gb:$daily_bandwidth_gb:normal" >> "$DB_FILE"
     
     local bw_display="Unlimited"
     if [[ "$bandwidth_gb" != "0" ]]; then bw_display="${bandwidth_gb} GB"; fi
@@ -1757,25 +1806,27 @@ edit_user() {
                if [[ -z "$new_pass" ]]; then
                    new_pass=$(tr -dc 'A-Za-z0-9' < /dev/urandom | head -c 8)
                    echo -e "${C_GREEN}🔑 Auto-generated: ${C_YELLOW}$new_pass${C_RESET}"
+               elif ! ff_is_valid_password "$new_pass"; then
+                   continue
                fi
                echo "$username:$new_pass" | chpasswd
-               sed -i "s/^$username:.*/$username:$new_pass:$cur_expiry:$cur_limit:$cur_bw:$cur_daily_bw/" "$DB_FILE"
+               db_set_user_field "$username" "$DB_FIELD_PASS" "$new_pass"
                echo -e "\n${C_GREEN}✅ Password for '$username' changed to: ${C_YELLOW}$new_pass${C_RESET}"
                ;;
             2) read -p "Enter new duration (in days from today): " days
                if [[ "$days" =~ ^[0-9]+$ ]]; then
                    local new_expire_date; new_expire_date=$(date -d "+$days days" +%Y-%m-%d); chage -E "$new_expire_date" "$username"
-                   sed -i "s/^$username:.*/$username:$cur_pass:$new_expire_date:$cur_limit:$cur_bw:$cur_daily_bw/" "$DB_FILE"
+                   db_set_user_field "$username" "$DB_FIELD_EXPIRY" "$new_expire_date"
                    echo -e "\n${C_GREEN}✅ Expiration for '$username' set to ${C_YELLOW}$new_expire_date${C_RESET}."
                else echo -e "\n${C_RED}❌ Invalid number of days.${C_RESET}"; fi ;;
             3) read -p "Enter new simultaneous connection limit: " new_limit
                if [[ "$new_limit" =~ ^[0-9]+$ ]]; then
-                   sed -i "s/^$username:.*/$username:$cur_pass:$cur_expiry:$new_limit:$cur_bw:$cur_daily_bw/" "$DB_FILE"
+                   db_set_user_field "$username" "$DB_FIELD_LIMIT" "$new_limit"
                    echo -e "\n${C_GREEN}✅ Connection limit for '$username' set to ${C_YELLOW}$new_limit${C_RESET}."
                else echo -e "\n${C_RED}❌ Invalid limit.${C_RESET}"; fi ;;
             4) read -p "Enter new TOTAL bandwidth limit in GB (0 = unlimited): " new_bw
                if [[ "$new_bw" =~ ^[0-9]+\.?[0-9]*$ ]]; then
-                   sed -i "s/^$username:.*/$username:$cur_pass:$cur_expiry:$cur_limit:$new_bw:$cur_daily_bw/" "$DB_FILE"
+                   db_set_user_field "$username" "$DB_FIELD_BW" "$new_bw"
                    local bw_msg="Unlimited"; [[ "$new_bw" != "0" ]] && bw_msg="${new_bw} GB"
                    echo -e "\n${C_GREEN}✅ Total bandwidth limit for '$username' set to ${C_YELLOW}$bw_msg${C_RESET}."
                    # Unlock user if they were locked due to bandwidth
@@ -1789,7 +1840,7 @@ edit_user() {
                else echo -e "\n${C_RED}❌ Invalid bandwidth value.${C_RESET}"; fi ;;
             5) read -p "Enter new DAILY bandwidth limit in GB (0 = unlimited): " new_daily_bw
                if [[ "$new_daily_bw" =~ ^[0-9]+\.?[0-9]*$ ]]; then
-                   sed -i "s/^$username:.*/$username:$cur_pass:$cur_expiry:$cur_limit:$cur_bw:$new_daily_bw/" "$DB_FILE"
+                   db_set_user_field "$username" "$DB_FIELD_DAILY_BW" "$new_daily_bw"
                    local daily_bw_msg="Unlimited"; [[ "$new_daily_bw" != "0" ]] && daily_bw_msg="${new_daily_bw} GB/day"
                    echo -e "\n${C_GREEN}✅ Daily bandwidth limit for '$username' set to ${C_YELLOW}$daily_bw_msg${C_RESET}."
                    # Unlock user if they were locked due to daily bandwidth
@@ -1993,11 +2044,7 @@ renew_user() {
     echo -e "\n${C_BLUE}🔄 Renewing selected users for $days days...${C_RESET}"
     for u in "${SELECTED_USERS[@]}"; do
         chage -E "$new_expire_date" "$u"
-        local line pass _expiry limit bw
+        db_set_user_field "$u" "$DB_FIELD_EXPIRY" "$new_expire_date"
-        line=$(grep "^$u:" "$DB_FILE")
-        IFS=: read -r _ pass _expiry limit bw _ <<< "$line"
-        [[ -z "$bw" ]] && bw="0"
-        sed -i "s/^$u:.*/$u:$pass:$new_expire_date:$limit:$bw/" "$DB_FILE"
         echo -e " ✅ ${C_YELLOW}$u${C_RESET} renewed until ${C_GREEN}${new_expire_date}${C_RESET}."
     done
 }
@@ -2311,8 +2358,9 @@ install_udp_custom() {
     else
         echo -e "${C_YELLOW}ℹ️ Architecture is $arch and no bundled arm64 udpgw was found. Compiling udpgw from source (needs internet, this may take a minute)...${C_RESET}"
         ff_pkg_install cmake g++ make git >/dev/null 2>&1
-        local temp_build="/tmp/badvpn_build"
+        # mktemp -d, not a fixed /tmp path: this is built and copied from as root.
-        rm -rf "$temp_build"
+        local temp_build
+        temp_build=$(mktemp -d) || return
         git clone -q https://github.com/ambrop72/badvpn.git "$temp_build"
         (cd "$temp_build" && cmake . >/dev/null 2>&1 && make >/dev/null 2>&1)
         local compiled_bin=$(find "$temp_build" -name "badvpn-udpgw" -type f | head -n 1)
@@ -4719,7 +4767,10 @@ create_trial_account() {
     # Password
     local password=$(tr -dc 'A-Za-z0-9' < /dev/urandom | head -c 8)
     read -p "🔑 Password [${password}]: " custom_pass
-    password=${custom_pass:-$password}
+    if [[ -n "$custom_pass" ]]; then
+        ff_is_valid_password "$custom_pass" || return
+        password="$custom_pass"
+    fi
     
     # Connection limit
     read -p "📶 Connection limit [1]: " limit
@@ -4749,10 +4800,15 @@ create_trial_account() {
     usermod -aG "$FF_USERS_GROUP" "$username" 2>/dev/null
     echo "$username:$password" | chpasswd
     chage -E "$expire_date" "$username"
-    echo "$username:$password:$expire_date:$limit:$bandwidth_gb:trial" >> "$DB_FILE"
+    # Keep the 7-field layout: the cleanup script reads the marker from field 7,
+    # so the daily-bandwidth field must be present even though trials do not set one.
+    echo "$username:$password:$expire_date:$limit:$bandwidth_gb:0:trial" >> "$DB_FILE"
 
     # Schedule auto-cleanup via 'at'
     echo "$TRIAL_CLEANUP_SCRIPT $username" | at now + ${duration_hours} hours 2>/dev/null
+    # Fallback for the limiter's trial sweep, in case atd is stopped or the job is lost.
+    mkdir -p "$BANDWIDTH_DIR"
+    date -d "+${duration_hours} hours" +%s > "$BANDWIDTH_DIR/${username}.trial_expiry"
     
     local bw_display="Unlimited"
     if [[ "$bandwidth_gb" != "0" ]]; then bw_display="${bandwidth_gb} GB"; fi