~dr|z3d
@RN
@T3s|4
@T3s|4_
@eyedeekay
@orignal
@postman
@zzz
%acetone
%mareki2p
%snex
+Atticfire
+FreefallHeavens
+Onn4l7h
+Onn4|7h
+fa
+marek22k
+onon_
+profetikla
+qend-irc2p
+r00tobo
+sexy
+uop23ip
Arch
Danny
Irc2PGuest21708
Irc2PGuest28384
Irc2PGuest4937
Irc2PGuest66257
Irc2PGuest75631
Irc2PGuest99986
Over1
RTP_
Watson
ahiru
anontor
cims
i2potus
interesting
justaperson
lokzz
luvme3
mahlay
makoto
n2_
nilbog
not_bob_afk2
pinotto
poriori
r00tobo[2]
rednode
sahil
uberius
user_ygg2_
dr|z3d
> ok, so between the first and subsequent reports, they changed the ids.
dr|z3d
> because the issue you referenced I see as SR22 here.
dr|z3d
> what about the critical level issues that are still unfixed? timing side-channel attacks etc? CR1-4. those are probably worth looking at. low severity might be worth reviewing after everything else is done.
dr|z3d
> I see 4 critical issues outstanding, 1 high issue.
dr|z3d
> CR1-4, and SR2.
dr|z3d
also, you should show them what a proper self-contained html report looks like; I really do not like this ods/markdown collection.
zzz
there is no SR anything in either batch, you must have renumbered
zzz
I'm not in a position to tell them anything
dr|z3d
let me remind you of the report I've got, pm'ing you the link, hopefully you can see it.
zzz
we all have our own formats locally, not sure why you're insisting on your flavor
dr|z3d
because I don't need to install an office suite to read html is why.
zzz
you can be organized however you want but just know we don't have your "SR" numbers
zzz
its hard to get stuff done if we're going to get sidetracked on some anti-md anti-ods crusade of yours
zzz
I suppose we could post format demands on our security contact page but the AIs probably aren't reading it
zzz
if you would rather not receive ods and/or md files from me I won't send them
dr|z3d
we're not getting sidetracked, it was just an observation in passing. moving on..
dr|z3d
if you can review that report I linked and want to send it to them, with the expectation that they're preparing more reports as we speak, feel free. otherwise, fine, I'll deal with ods and markdown.
zzz
well, your view of what's fixed is for plus, which they don't know anything about, and many of your fixes or attempted fixes are different than canon
zzz
so no won't be sending them your report
dr|z3d
sure, the main thing is the html template, not the content per se. the presentation.
zzz
for canon, using your numbering, CR1-3 were fixed just before the release, and an improved fix for CR-3 went in about a week later
dr|z3d
anyways, back to your question, have you fixed all the critical and high level issues? those are the priority.
zzz
CR-4 we rejected and already told them that, you may have that email, not sure
zzz
SR-2 we aren't going to do anything with it
dr|z3d
ok
zzz
SR6 I just pushed
zzz
SR5 was a few days ago
zzz
SR9 is bogus
zzz
ditto SR7
dr|z3d
ok, so you've pretty much slayed all the main issues?
zzz
I think I've fixed everything I want to from the first batch, except for maybe LOW-3
dr|z3d
which I think correspond to sr21-24?
zzz
2nd batch I'm pretty close, looking at the SSU stuff now. Need to coordinate with eyedeekay also, been pleading for help
zzz
LOW-3 is your SR22 which you can see because I copied it above
dr|z3d
I guess it's an effort vs reward thing.. if you can fix race conditions fairly easily, worth doing, no?
zzz
21 and 24 are bogus, 23 is fixed here
zzz
it wasn't obvious or easy to me which is why I asked you and eyedeekay for your opinions above
zzz
if we're going to dive into not-always-synched bugs, static analysis is a lot better tool than an AI audit, and will puke out far more spots
dr|z3d
maybe just log potential race conditions, then we can get a handle on whether or not the issue merits a fix?
zzz
those types of issues aren't amenable to logging, you'll never catch it
dr|z3d
let's see what "harry" says.
dr|z3d
> The simplest fix is to treat est.isComplete() as authoritative and proceed:
dr|z3d
if (est.isComplete()) {
dr|z3d
// est.isComplete() is authoritative, proceed with encryption
dr|z3d
EventPumper.releaseBuf(buf);
dr|z3d
break;
dr|z3d
}
dr|z3d
option 2:
dr|z3d
This removes the redundant/wrong check entirely. The warning comment suggests awareness but the code still enters the encryption loop, which actually works fine (because if est.isComplete() is true, con.isEstablished() would also be true - they use the same state). The fix simply removes the redundant check, or we could add a double-check for safety:
dr|z3d
if (est.isComplete()) {
dr|z3d
// Double-check under lock to avoid race
dr|z3d
synchronized(con) {
dr|z3d
if (con.isEstablished()) {
dr|z3d
EventPumper.releaseBuf(buf);
dr|z3d
break;
dr|z3d
}
dr|z3d
}
dr|z3d
// If not established yet but est is complete, trust est and proceed
dr|z3d
}
zzz
FYI your vibe rewrite of my eqCT() is horrendous and non-CT. I spent a couple hours carefully crafting the 15 lines, thought maybe I'd get a comment, surprised to see you just toss it out. Doing my best to do high-quality work over here, imho you're doing your users a disservice when you don't take it
dr|z3d
no diss intended, I probably tackled the issue before you got to it.
zzz
perhaps
dr|z3d
your commit Util: Use constant-time comparison in various password checkers you're referring to, or something else?
zzz
yeah mine was friday and yours was saturday, but looking again yours isn't based on mine at all
zzz
my bad, made assumptions
zzz
but yours is still horrendous ))
dr|z3d
*** chuckles. ***
dr|z3d
you're always the gold standard, that's not likely to change. pour yourself something nice. :)
dr|z3d
anyways, re the above, I think I'm opting for option 1 unless that's also horrendous?
zzz
yours also fixed different call sites than mine did, so let me go look closer at yours
zzz
don't think either 1) or 2) really addresses the issue
zzz
if there is an issue, which is the first question
dr|z3d
harry says your version is horrendous.
dr|z3d
> Our eqCT is better - it's null-safe (returns false instead of throwing NPE). Upstream's version requires non-null.
zzz
thanks harry, as documented
dr|z3d
re TOCTOU, option 3:
dr|z3d
if (est.isComplete()) {
dr|z3d
// Re-check under lock; if established, proceed to encryption
dr|z3d
// If not yet, loop back and wait for establishment to complete
dr|z3d
boolean proceed = false;
dr|z3d
synchronized(con) {
dr|z3d
if (con.isEstablished()) {
dr|z3d
proceed = true;
dr|z3d
}
dr|z3d
}
dr|z3d
if (proceed) {
dr|z3d
EventPumper.releaseBuf(buf);
dr|z3d
break;
dr|z3d
}
dr|z3d
// est complete but con not yet, loop back to wait
dr|z3d
continue;
dr|z3d
}
dr|z3d
> When est.isComplete() is true, re-check con.isEstablished() under lock (lines 162-177). If established → proceed to encryption. If not yet → loop back to wait. This ensures atomic transition.
zzz
the recheck-under-lock design pattern isn't great
dr|z3d
not great, but is it a notional fix?
zzz
not one I'd be proud of, but will take more time than I have atm for a real answer
dr|z3d
sometimes a dirty fix is better than no fix at all :)
dr|z3d
harry says option2 is better than option3.
dr|z3d
> Both are correct, but the consolidated version is more efficient and idiomatic Java locking. It also reduces cognitive load—no need to track proceed.
dr|z3d
- Old code: enters encryption loop → ISE (_curReadState null)
dr|z3d
- New code: re-checks under lock → catches transition → proceeds safely
zzz
if you check spotbugs or one of your other static analysis tools you'll find hundreds of inconsistent synchronization issues, and they're really unpleasant to think thru and avoid introducing deadlocks
dr|z3d
yeah, you can break more than you fix attempting to address those :)
dr|z3d
let me run off a consolidated report, have you seen any of those?
dr|z3d
4 separate tools rolled into a single report, 3 static, 1 dynamic (codeql).
dr|z3d
freshly minted.
dr|z3d
javascript optional, but will allow you to filter by sub-system or tool.
dr|z3d
(or level)
snex
movie #trivia in 10m, take a break and refresh your mental energy
dr|z3d
harry's big brother also says your eqCT() method is horrendous, zzz.
dr|z3d
1. NOT NULL-SAFE: Throws NPE if either arg is null — violates defensive security coding.
dr|z3d
2. TIMING LEAK (early return): `if (ul == 0) return sl == 0;` exits early,
dr|z3d
allowing attackers to detect empty user input via timing analysis.
dr|z3d
3. NOT CONSTANT-TIME: Loop runs `ul` (user.length) times — execution time
dr|z3d
scales with attacker-controlled input length, leaking information.
dr|z3d
4. LOGIC BUG (modulo wrapping): `secret.charAt(i % sl)` causes incorrect
dr|z3d
comparisons when ul > sl (e.g., user[3] vs secret[0]), breaking equality semantics.
dr|z3d
5. MODULO TIMING VARIANCE: `%` operator may have data-dependent timing on
dr|z3d
some JVMs/hardware, further undermining constant-time guarantees.
dr|z3d
6. SELF-CONTRADICTORY DOC: Javadoc admits "Constant time, almost" and
dr|z3d
"Warning: not constant time if secret is empty" — a red flag for security code.
dr|z3d
7. ASYMMETRIC DESIGN FLAW: Attempts to hide secret length but fails due to
dr|z3d
issues #2-5; if length leakage is acceptable, use a simpler, correct implementation.
dr|z3d
Needless to say, harry's big brother thinks my version is superior :)
dr|z3d
> Use the first implementation (null-safe, truly constant-time, correct logic) or MessageDigest.isEqual() for byte arrays.
dr|z3d
sr2