session server ssh FEATURE lock an account out after repeated failed password authentication by niklas-moser · Pull Request #640 · CESNET/libnetconf2

niklas-moser · 2026-09-07T11:00:26Z

Nothing reads ssh_auth_attempts, so password guessing is unlimited both within a connection and across them. auth-timeout bounds how long one authentication may take, not how many may be tried, and pam_faillock only ever sees the
keyboard-interactive method — accounts using hashed-password are verified by libnetconf2 itself with crypt(3).

This counts consecutive password failures per account across connections and refuses the account for NC_AUTHLOCK_TIME after NC_AUTHLOCK_MAX_FAILS. The hooks sit on the three credential checks in session_server_ssh.c that both the message-and callback-based backends funnel through (auth_password_check, kbdint_verify_passwd, pam_authenticate), so all three methods share one tally and neither dispatch file is touched. The tally is mirrored to a state file and re-read when it changes, so a lockout survives a restart and can be cleared on a running server. A session hitting NC_AUTHLOCK_SESSION_MAX_FAILS is disconnected — ssh_auth_attempts finally gets a reader.

Public key auth is deliberately not counted, which keeps a locked out deployment recoverable. TLS is unaffected.

Open question: the policy is compiled in (5 failures, 300 s, 900 s window, 6 per session, 64 accounts). ietf-netconf-server has no leaves for it and your augment carries only auth-timeout, so making it configurable means extending
libnetconf2-netconf-server.yang. If you want to make it configurable, we can change the approach there

Motivated by O-RAN WG11 R004 / 3GPP TS 33.117 4.2.3.4.3.1.

…password authentication Nothing reads ssh_auth_attempts, so password guessing is unlimited both within a connection and across them. auth-timeout bounds how long one authentication may take, not how many may be tried, and pam_faillock only ever sees the keyboard-interactive method. Count consecutive password failures per account across connections and refuse the account for NC_AUTHLOCK_TIME after NC_AUTHLOCK_MAX_FAILS. The hooks sit on the three credential checks in session_server_ssh.c that both auth backends funnel through, so the configured-password, kbdint and PAM methods share one tally and neither dispatch file is touched. The tally is mirrored to a state file and re-read when it changes, so a lockout survives a restart and can be cleared on a running server. A session hitting NC_AUTHLOCK_SESSION_MAX_FAILS is disconnected. Public key auth is deliberately not counted, which keeps a locked out deployment recoverable. The policy is compiled in; ietf-netconf-server has no leaves for it.

Roytak

Thanks for the contribution and sorry for the delay! I think this feature is very useful, but will need a bit more work - mainly making this optional (off by default) and configurable. Also could add at least one test e.g. for locking an account out after N attempts.

Roytak · 2026-09-16T14:17:04Z

+ * @param[in] success Whether it succeeded, which clears the tally.
+ */
+static void
+nc_authlock_record(struct nc_session *session, const char *username, int success)


The problem I have with this is that the tally is keyed on the username alone, and the username is entirely attacker-controlled. Anyone who can reach the NETCONF port can keep root/admin permanently locked out of password auth by sending 5 bad passwords every 300 s. At minimum the lockout should be keyed by (username, peer address), and the deployment must be able to turn it off. For the peer address I believe you can use session->host, but it may be NULL...

Agreed, this is a real denial of service and I will key the tally on (username, peer address), but I would keep the per-account component rather than replace it, since peer-only keying leaves a distributed guesser unbounded and 3GPP TS 33.117 4.2.3.4.3.1 asks for the account-level lockout specifically.

Roytak · 2026-09-16T14:22:31Z

+#define NC_AUTHLOCK_MAX_FAILS 5             /**< consecutive failures that lock an account out */
+#define NC_AUTHLOCK_TIME 300                /**< how long an account stays locked out, seconds */
+#define NC_AUTHLOCK_FAIL_INTERVAL 900       /**< failures further apart than this start a new tally, seconds */


Every existing libnetconf2 user gets a behaviour change they cannot configure, disable, or even observe except through a WRN. Add an augment to the libnetconf2-netconf-server YANG with the leaves (e.g. max_fails, lock_time, window probably somewhere under SSH's client-authentication) and default to disabled, so this is opt-in.

I will add the augment to libnetconf2-netconf-server under SSH client-authentication with the policy leaves and the feature disabled by default.

Roytak · 2026-09-16T14:25:56Z

+    if (authlock.entry_count < NC_AUTHLOCK_MAX_ENTRIES) {
+        slot = authlock.entry_count++;
+    } else {
+        /* full, reuse the entry that failed longest ago. With local users configured only accounts
+         * known to the endpoint reach the authentication dispatch, so this bound is about memory
+         * rather than about an attacker flooding the tally with invented names. */
+        slot = 0;
+        for (i = 1; i < authlock.entry_count; ++i) {
+            if (authlock.entries[i].last_fail < authlock.entries[slot].last_fail) {
+                slot = i;
+            }
+        }
+        free(authlock.entries[slot].username);
+    }


You evict the entry with the oldest last_fail when the table is full here, without regard for whether that entry is currently locked. I think you should never evict an entry with locked_until > now, prefer evicting expired/unlocked entries and if none are available, refuse to add rather than dropping a lockout.

Makes sense, thanks

Roytak · 2026-09-16T14:29:45Z

+static const char *
+nc_authlock_path(void)
+{
+    const char *path = getenv("NC_AUTHLOCK_FILE");
+
+    return (path && path[0]) ? path : NC_AUTHLOCK_FILE;
+}


Nothing in the build creates /var/lib/netconf-authlock/, so on a normal install every open() fails, a single WRN is logged, and the persistence half of the feature is silently absent.

Make the default path a CMake option (off by default), expose a setter in the public API, drop the getenv, and create the directory (or log loudly and disable persistence).

Roytak · 2026-09-16T14:32:20Z

+    /* free the password authentication lockout tally, the state file it mirrors is kept */
+    nc_server_ssh_authlock_free();


It sits before nc_server_ch_threads_destroy() so Call Home and accept threads are still authenticating clients. Move the call to the end of the teardown, after all threads are joined.

Roytak · 2026-09-16T14:43:22Z

+        ERR(session, "Too many failed authentication attempts (%d) in a single session, disconnecting.",
+                session->opts.server.ssh_auth_attempts);
+        NC_SESSION_STATUS_SET(session, NC_STATUS_INVALID);
+        ATOMIC_STORE_RELAXED(session->term_reason, NC_SESSION_TERM_OTHER);


Use NC_SESSION_TERM_REASON_SET.

Roytak · 2026-09-16T14:44:53Z

+            WRN(session, "User \"%s\" locked out of password authentication for %d s after %u consecutive "
+                    "failed attempts.", username, NC_AUTHLOCK_TIME, entry->fails);


Use PRIu32 instead of %u (3 occurences I believe).

Roytak · 2026-09-16T14:45:31Z

+
+    /* the username is the rest of the line so that it may contain spaces */
+    while ((authlock.entry_count < NC_AUTHLOCK_MAX_ENTRIES) && fgets(line, sizeof line, f)) {
+        if (sscanf(line, "%u %lld %lld %n", &fails, &locked_until, &last_fail, &offset) != 3) {


Should use SCNu32 and uint32_t fails.

Roytak · 2026-09-16T14:46:40Z

+struct nc_authlock_entry {
+    char *username;             /**< account the tally belongs to */
+    uint32_t fails;             /**< consecutive failed password authentications */
+    time_t last_fail;           /**< when the last one was */
+    time_t locked_until;        /**< no password authentication before this, 0 if not locked out */
+};
+
+static struct {
+    pthread_mutex_t lock;
+    struct nc_authlock_entry entries[NC_AUTHLOCK_MAX_ENTRIES];
+    uint32_t entry_count;
+    int loaded;                 /**< whether the state file was read already */
+    ino_t loaded_ino;           /**< inode of the state file revision the tally came from */
+    struct timespec loaded_mtim; /**< mtime of the state file revision the tally came from */
+    int store_failed;           /**< whether a failure to write the state file was logged already */
+} authlock = {.lock = PTHREAD_MUTEX_INITIALIZER};


Should be moved to session_p.h along with any needed macros.

Roytak · 2026-09-16T14:47:08Z

+/**
+ * @brief Free the password authentication lockout tally.
+ *
+ * The state file it mirrors is kept, so a lockout survives the server being restarted.
+ */
+void nc_server_ssh_authlock_free(void);


Belongs to session_p.h imo.

nc_server_ssh_authlock_free() sat before nc_server_ch_threads_destroy(), so the Call Home and accept threads were still authenticating clients against a table whose username strings had already been freed. Move the call to the last teardown step, once every thread that can reach the tally has been joined. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…t-in Key the tally on the client address too, configure the policy through YANG with the lockout off by default, and make the state file an AUTHLOCK_FILE CMake option. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

niklas-moser · 2026-09-18T09:58:37Z

Thank you @Roytak . I added two commits for you to review regarding the requested changes. If you want I can squash them after re-review, or you can do it.

Roytak

Needs some minor changes + 1 bigger one to nc_authlock_get. Also needs a rebase on top of devel due to a conflict. Otherwise okay.

Roytak · 2026-09-21T08:10:43Z

+        entry->locked_until = locked_until;
+        entry->last_fail = last_fail;


Could probably add a sanity check just in case, something like:
locked_until > now + NC_AUTHLOCK_TIME -> now + NC_AUTHLOCK_TIME
last_fail > now -> now

Roytak · 2026-09-21T08:15:51Z

+      }
+
+      leaf lock-time {
+        type uint16;


Should also add range "1..max".

Roytak · 2026-09-21T08:15:59Z

+      }
+
+      leaf fail-window {
+        type uint16;


Should also add range "1..max".

Roytak · 2026-09-21T08:18:15Z

+static struct {
+    pthread_mutex_t lock;
+    struct nc_authlock_entry entries[NC_AUTHLOCK_MAX_ENTRIES];
+    uint32_t entry_count;
+    char *path;                 /**< state file the tally is mirrored to, NULL to keep it in memory only */
+    int path_set;               /**< whether @p path was resolved already */
+    int loaded;                 /**< whether the state file was read already */
+    ino_t loaded_ino;           /**< inode of the state file revision the tally came from */
+    struct timespec loaded_mtim; /**< mtime of the state file revision the tally came from */
+    int store_failed;           /**< whether the state file turned out to be unusable, disables persistence */
+} authlock = {.lock = PTHREAD_MUTEX_INITIALIZER};


For consistency, give this struct a name, doxygen docs, and move its definition to session_p.h. Keep the global variable definition here afterwards.

Roytak · 2026-09-21T08:41:31Z

+        if (slot == NC_AUTHLOCK_MAX_ENTRIES) {
+            /* every tracked pair is locked out, so refuse to track another one rather than drop a
+             * lockout; the pairs that are locked out keep being refused either way */
+            WRN(NULL, "Authentication failure tally full of locked out users, not counting the "
+                    "failures of user \"%s\" for now.", username);
+            return NULL;


This still looks easily abusable. I would recommend something along:

add NC_AUTHLOCK_MAX_ENTRIES_PER_HOST and set to e.g. 8

add host_count = after nc_authlock_find misses in this function

change authlock.entry_count < NC_AUTHLOCK_MAX_ENTRIES to (authlock.entry_count < NC_AUTHLOCK_MAX_ENTRIES) && (host_count < NC_AUTHLOCK_MAX_ENTRIES_PER_HOST)

in the for loop above add another if case, e.g. if (!nc_authlock_same_host(authlock.entries[i].host, host)) and continue in that case.

instead of WRN and fail, evict the last/min last fail

Also in this file on line :1050, drop the PAM_USER_UNKNOWN - mirrors the shadow/local-users path. Also you can possibly just prune the table while walking it in the for above, i.g. entries with locked_until <= now and last_fail older than fail_window are dead weight.

Roytak · 2026-09-21T08:50:06Z

+        "Locks an account out of password authentication once it fails too many times in a row.
+         Failures are counted per (username, client address) pair across connections, so that a
+         client that cannot reach the account from its own address cannot lock the account out for
+         anyone else. The tally is shared by every password-based method the server offers, which
+         are the configured password, keyboard-interactive and PAM. Public key authentication is
+         not counted and never refused, so a locked out deployment stays reachable.
+
+         The policy configured here is the one of the endpoint the connection arrived on; it is not
+         re-evaluated when authentication falls through to a referenced endpoint.
+
+         The tally is kept in memory. It additionally survives a server restart if the server was
+         built with the AUTHLOCK_FILE option or the application set a path with the
+         nc_server_ssh_set_authlock_path() API call.";


Overall pretty long. No need to mention implementation details in the YANG description, so the last paragraph + (username, client address) part can be dropped. Other parts possibly too.
It should plainly metion something along: "it's an account-lockout control for 3GPP 33.117 compliance, that the brute-force rate limit is max-auth-attempts + auth-timeout, and that enabling lockout without max-auth-attempts still enables one connection to make an unlimited amount of attempts".

niklas-moser force-pushed the ssh-auth-lockout branch from 20c3063 to dccb1e7 Compare September 16, 2026 07:21

Roytak requested changes Sep 16, 2026

View reviewed changes

niklas-moser and others added 2 commits September 18, 2026 11:54

Roytak requested changes Sep 21, 2026

View reviewed changes

		/* free the password authentication lockout tally, the state file it mirrors is kept */
		nc_server_ssh_authlock_free();

		WRN(session, "User \"%s\" locked out of password authentication for %d s after %u consecutive "
		"failed attempts.", username, NC_AUTHLOCK_TIME, entry->fails);

		entry->locked_until = locked_until;
		entry->last_fail = last_fail;

Conversation

niklas-moser commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Roytak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklas-moser commented Sep 18, 2026

Uh oh!

Roytak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

niklas-moser commented Sep 7, 2026 •

edited

Loading