Repository navigation
Separate SESS-06/SESS-07 scope and add absolute timeout testing - #1587
enesbaldemir wants to merge 2 commits into
Conversation
Signed-off-by: enesbaldemir <enesbaldemir9538@gmail.com>
|
Here's some suggestions from Grok that seem worth addressing:
|
fzipi
left a comment
fzipi
left a comment
There was a problem hiding this comment.
Review findings (Claude Code, high effort), checked against style_guide.md and template/.
| 3. Continue past a reasonable maximum lifetime for the application (see the Summary for typical values). | ||
| 4. The application should eventually require the user to authenticate again, even though the session was never idle. If the session stays valid for as long as it keeps being used, no absolute timeout is enforced. | ||
|
|
||
| If the session lifetime appears to be tied to a value the client controls, such as the `Expires` or `Max-Age` attribute of a cookie or a timestamp inside a token (for example the `exp` claim of a [JSON Web Token](10-JSON_Web_Tokens.md)), check whether modifying it extends the session. As with the idle timeout, the limit must be enforced server-side. |
There was a problem hiding this comment.
The new client-controlled-lifetime check tells testers to modify a cookie's Expires/Max-Age or a JWT exp claim and see whether the session is extended, but neither manipulation can produce the described result.
Cookie Expires/Max-Age are browser-side attributes that are never sent to the server, so editing them cannot extend a server-side session; the meaningful test is replaying the cookie value after its expiry time has passed. Editing exp in a correctly signed JWT just breaks the signature (401), which a tester following this text would misread as 'limit enforced server-side'; the real check is replaying the token after exp and probing the refresh-token lifetime. The idle paragraph at line 36 at least conditions this on 'if it's not cryptographically protected'; the new paragraph drops that caveat.
|
|
||
| 1. Log in and note the time. | ||
| 2. Keep the session active by sending a request to an authenticated page at intervals shorter than the idle timeout, for example with [Burp Repeater](https://portswigger.net/burp/documentation/desktop/tools/repeater) or a simple script. | ||
| 3. Continue past a reasonable maximum lifetime for the application (see the Summary for typical values). |
There was a problem hiding this comment.
The Absolute Timeout black-box procedure has no stopping criterion and step 4 draws an absolute conclusion ('no absolute timeout is enforced') from a finite observation.
Step 3 says 'continue past a reasonable maximum lifetime (see the Summary)', but the Summary only offers 'a few hours' or 'a working day'. A tester who keeps the session alive for 6 hours cannot distinguish a 12-hour or 24-hour absolute timeout from none, yet the text instructs them to report that none is enforced. The step should state a concrete window and a hedged conclusion ('lifetime exceeds N hours') and point to the gray-box/config check for confirmation.
| - The log out function effectively destroys all session token, or at least renders them unusable, | ||
| - The server performs proper checks on the session state, disallowing an attacker to replay previously destroyed session identifiers | ||
| - A timeout is enforced and it is properly enforced by the server. If the server uses an expiration time that is read from a session token that is sent by the client (but this is not advisable), then the token must be cryptographically protected from tampering. | ||
| - An idle timeout is enforced by the server, based on the time since the last request in the session. |
There was a problem hiding this comment.
Gray-Box list drops the check that the server rejects replay of previously destroyed session identifiers, which for SESS-07 means expired (timed-out) IDs, not just logged-out ones.
The removed bullet 'The server performs proper checks on the session state, disallowing an attacker to replay previously destroyed session identifiers' was treated as a logout duplicate, but after an idle/absolute timeout fires it is the core SESS-07 gray-box check. The new list only verifies that timeouts exist and are server-based; an implementation that flags a session as expired yet still accepts the old ID on replay satisfies all three new bullets. Line 57 mentions invalidate()/abandon() but never states that expired IDs must be rejected.
|
|
||
| - Availability of user interface controls that allow the user to manually log out. | ||
| - Session termination after a given amount of time without activity (session timeout). | ||
| - Session termination after a given amount of time without activity (session timeout), covered in [Session Timeout](07-Session_Timeout.md). |
There was a problem hiding this comment.
The 'secure session termination requires at least the following components' list still names only the inactivity timeout, while SESS-07 now states applications 'should also enforce an absolute timeout'.
The two sections now disagree on the required components: a reader of SESS-06 is told the minimum is a logout control, an idle timeout, and server-side invalidation, with no mention of the absolute lifetime limit that SESS-07 adds as a requirement in the same PR. Either the bullet should read 'automatic expiration (idle and absolute timeouts)' or point generically to SESS-07 without enumerating.
| 3. Continue past a reasonable maximum lifetime for the application (see the Summary for typical values). | ||
| 4. The application should eventually require the user to authenticate again, even though the session was never idle. If the session stays valid for as long as it keeps being used, no absolute timeout is enforced. | ||
|
|
||
| If the session lifetime appears to be tied to a value the client controls, such as the `Expires` or `Max-Age` attribute of a cookie or a timestamp inside a token (for example the `exp` claim of a [JSON Web Token](10-JSON_Web_Tokens.md)), check whether modifying it extends the session. As with the idle timeout, the limit must be enforced server-side. |
There was a problem hiding this comment.
New paragraph re-implements the idle section's client-vs-server enforcement test (line 36) with slightly different wording instead of sharing one explanation.
Lines 36-38 already say: if the cookie carries time data, modify it and see whether the session is prolonged; everything must be checked server-side. Line 47 repeats this for the absolute case (cookie Expires/Max-Age, token timestamp, 'must be enforced server-side'), and the gray-box bullet at line 55 says it a third time. Two divergent copies of the same procedure will drift; a single 'Client vs. Server Enforcement' paragraph after both subsections covers both timeouts.
| Some web application frameworks rely solely on the session cookie to identify the logged-on user. The user's ID is embedded in the (encrypted) cookie value. The application server does not do any tracking on the server-side of the session. When logging out, the session cookie is removed from the browser. However, since the application does not do any tracking, it does not know whether a session is logged out or not. So by reusing a session cookie it is possible to gain access to the authenticated session. A well-known example of this is the Forms Authentication functionality in ASP.NET. | ||
|
|
||
| Users of web browsers often don't mind that an application is still open and just close the browser or a tab. A web application should be aware of this behavior and terminate the session automatically on the server-side after a defined amount of time. | ||
| Users of web browsers often don't mind that an application is still open and just close the browser or a tab. A web application should be aware of this behavior and terminate the session automatically on the server-side after a defined amount of time. Testing for this is covered in [Session Timeout](07-Session_Timeout.md). |
There was a problem hiding this comment.
The Summary now links to 07-Session_Timeout.md twice (line 14 and line 23) for the same purpose.
Issue #1586 asked for 'a one-line pointer to SESS-07'. Both the component bullet and the browser-close paragraph now end with 'covered in [Session Timeout]'; one of the two pointers is redundant, and the browser-close paragraph itself is timeout content that the PR's stated goal ('logout only') would move or drop.
|
|
||
| All applications should implement an idle or inactivity timeout for sessions. This timeout defines the amount of time a session will remain active in case there is no activity by the user, closing and invalidating the session upon the defined idle period since the last HTTP request received by the web application for a given session ID. The most appropriate timeout should be a balance between security (shorter timeout) and usability (longer timeout) and heavily depends on the sensitivity level of the data handled by the application. For example, a 60 minute log out time for a public forum can be acceptable, but such a long time would be too much in a home banking application (where a maximum timeout of 15 minutes is recommended). In any case, any application that does not enforce a timeout-based log out should be considered not secure, unless such behavior is required by a specific functional requirement. | ||
|
|
||
| The idle timeout limits the chances that an attacker has to guess and use a valid session ID from another user, and under certain circumstances could protect public computers from session reuse. However, if the attacker is able to hijack a given session, the idle timeout does not limit the attacker’s actions, as he can generate activity on the session periodically to keep the session active for longer periods of time. |
There was a problem hiding this comment.
Third-person 'he' remains in the Summary paragraph directly above the inserted absolute-timeout paragraph, violating the style guide's Second Person rule, while the PR removed the equivalent 'him/her' two paragraphs below.
style_guide.md 'Second Person': 'Do not write in the first or third person, such as by using I or he.' Line 13 reads 'as he can generate activity on the session periodically'. The PR edited this Summary and deleted 'impersonate him/her' from it but left this instance; rewrite as 'as the attacker can generate activity'.
|
|
||
| When either timeout expires, the session must be terminated in the same way as on log out: all session tokens (e.g. cookies) must be destroyed or made unusable server-side, so that they cannot be replayed to "resurrect" the session of a legitimate user. See [Logout Functionality](06-Logout_Functionality.md) for details. | ||
|
|
||
| The most common scenario for this kind of attack is a public computer that is used to access some private information (e.g., web mail, online bank account). If the user moves away from the computer without explicitly logging out and the session timeout is not implemented on the application, then an attacker could access to the same account by simply pressing the "back" button of the browser. |
There was a problem hiding this comment.
'The most common scenario for this kind of attack' has lost its antecedent after the surrounding paragraphs were rewritten and reordered.
Originally the sentence followed the full cookie-replay paragraph. It now follows the absolute-timeout, server-side-enforcement, and one-sentence replay paragraphs, and describes a public-computer/back-button scenario that is an idle-timeout concern. A reader takes 'this kind of attack' to mean the absolute-timeout/hijack case just introduced, which the public-computer example does not illustrate; the paragraph belongs after the idle-timeout paragraph (line 11) or should name the attack explicitly.
| The idle timeout limits the chances that an attacker has to guess and use a valid session ID from another user, and under certain circumstances could protect public computers from session reuse. However, if the attacker is able to hijack a given session, the idle timeout does not limit the attacker’s actions, as he can generate activity on the session periodically to keep the session active for longer periods of time. | ||
|
|
||
| Session timeout management and expiration must be enforced server-side. If some data under the control of the client is used to enforce the session timeout, for example using cookie values or other client parameters to track time references (e.g. number of minutes since log in time), an attacker could manipulate these to extend the session duration. So the application has to track the inactivity time server-side and, after the timeout is expired, automatically invalidate the current user's session and delete every data stored on the client. | ||
| This is why applications should also enforce an absolute timeout, which limits the total lifetime of a session regardless of activity. Once it expires, the user has to authenticate again even if the session was never idle. Its value also depends on the sensitivity of the application, for example a few hours for a banking application or a working day for an internal business application. |
There was a problem hiding this comment.
The recommended absolute-timeout values ('a few hours for a banking application') are unsourced and do not match the cited Session Management Cheat Sheet, which the Absolute Timeout step 3 then relies on as 'typical values'.
The cheat sheet section the PR links (#automatic-session-expiration) gives 4-8 hours for a full-day office application and no banking figure. The WSTG text invents 'a few hours' for banking and 'a working day' for internal apps, and step 3 (line 44) instructs testers to use these as pass/fail thresholds. Testers will flag an app with a 4-hour banking lifetime inconsistently depending on how they read 'a few'.
| #### Absolute Timeout | ||
|
|
||
| 1. Log in and note the time. | ||
| 2. Keep the session active by sending a request to an authenticated page at intervals shorter than the idle timeout, for example with [Burp Repeater](https://portswigger.net/burp/documentation/desktop/tools/repeater) or a simple script. |
There was a problem hiding this comment.
Burp Repeater is referenced inline as 'Burp Repeater' while the sibling SESS-06 lists the same URL under a ## Tools section as 'Burp Suite - Repeater'; SESS-07 has no Tools section.
Every other section in this chapter that names a tool (01, 02, 03, 05, 06, 09, 10) also lists it under ## Tools. SESS-07 now names a tool but has no Tools section, and uses a different display name than SESS-06 for the identical link, so a reader scanning Tools sections for Burp usage will miss SESS-07 and the two sections will drift when the URL changes.
|
@enesbaldemir Feel feee to respond if you think any AI comments are off base. |
Signed-off-by: enesbaldemir <enesbaldemir9538@gmail.com>
|
@kingthorin I went through each point and they all looked valid to me, so I addressed all of them rather than pushing back on any. Thanks for the thorough review, @fzipi! All points are addressed in the latest commit (8c534d4):
|
|
@kingthorin The Grok suggestions are covered in the same commit as well: the absolute timeout test window and hedged conclusion, server-side invalidation in step 4, a gray-box check for the configured timeout values / session store TTL, the Tools section, and a short SSO note. The trailing space before the period wasn't present in the file. |
|
Thanks! I'll get back to it tomorrow (I hope). |
Fixes #1586
What did this PR accomplish?
Gives SESS-06 and SESS-07 each a clear scope, removes the overlap between them, and adds absolute timeout testing. Both IDs are unchanged.
WSTG-SESS-06 (Logout Functionality), now logout only
### Session Timeoutsubsection, which duplicated SESS-07. Its two typos ("excepted", "In a banking applications") go with it.WSTG-SESS-07 (Session Timeout), now all automatic expiration
#### Idle Timeout. There's also a new#### Absolute Timeouttest: keep the session active with periodic requests past a reasonable maximum lifetime, confirm re-authentication is required, and check the limit isn't tied to a client-controlled value (cookieExpires/Max-Age, JWTexp).markdownlint-cli2and the terminologytextlintcheck pass on both files. Nothing else links to removed headings, and the checklist entries will pick up the new SESS-06/07 objectives on the next checklist publish.