On Thu, Sep 05, 2019 at 10:59:46AM -0400, Greg Veldman wrote:

On Thu, Sep 05, 2019 at 11:52:45AM +0200, Ond??ej Kuzn??k wrote:

...

• could you split it in two patches, one to check the previous time step (+doc) and one to support the new schemes (+doc)?

Working on it, will have updated patches up shortly...

Thanks for the updated patches.

...

• I don't think you need to allocate a copy of the passwd just come in, you can just frame it into separate bervals reusing the provided buffer so long as you keep in mind they are not NUL-terminated properly.

Are you referring to the chk_totp_and_pw() function? If so, since the expected format is <password><totp> with no seperator, if I terminated the password part that would overwrite the first char of totp, yes? That's the reason I make a copy and allocate an extra byte for the NUL.

I mean the ber_str2bv(,,1,) in both new functions. Not sure which code you think would overwrite parts of the buffer? ber_str2bv(,,0,) never touches it, manually initialising the berval certainly wouldn't either. And then you have fewer memory regions to scrub.

Since you already know the length, you can also pass it in so ber_str2bv can skip its strlen() check (and since anything can be in a {PLAINTEXT} password, you're now embedded NUL safe).

...

Just a style note, if there's an else coming, could you make sure both the if and the else blocks are in {}?

Implemented, it will be included in the updated patches.

I think I mentioned this before as something worth changing: rather than call time(0L), you can use op->o_time which is stable and the closest you can get to the time the operation was received.

BTW, scinet.supercomputing.org's HTTPS cert is signed by Let's Encrypt Authority X3 as an intermediate, but isn't sending it during the handshake, so wget/curl aren't happy trusting it (I think browsers cache it or already have a copy).

Thanks,