Nixpkgs: pbkbfd: Use getdelim instead of fgets

[0xABAB]

https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/system/boot/pbkdf2-sha512.c#L28 should be using the POSIX 2008 http://pubs.opengroup.org/onlinepubs/9699919799/functions/getdelim.html instead of fgets.

If, for some reason there are targets that do not have POSIX 2008, add configuration flags to also accommodate for non-POSIX 2008 systems, develop a NixOS module to control this behavior with sane defaults set depending on the platform (which also should be integrated with the installer eventually). (Feel free to create other tickets for these tasks, if need arises.)

@Calrama

Version: master

[MoritzMaxeiner]

I don't use NixOS/nixpkgs anymore, so feel free do do what you want, but with regards to your claim that it should not use "brittle" C functions: you have to substantiate such claims, i.e. prove what supposedly makes fgets (which has well defined behaviour) "brittle" in this specific context.

@0xABAB

[joachifm]

Seems to me that if the user enters a NUL, the current code will ignore everything the user enters after the NUL (strlen stops at first NUL, whereas fgets is happy to continue reading until newline). If users feed binary data to this program, the input key passed to the kdf may be substantially shorter than expected.

No idea if there are any practical implications re how this program is intended to be used, though.

[MoritzMaxeiner]

Seems to me that if the user enters a NUL, the current code will ignore everything the user enters after the NUL (strlen stops at first NUL, whereas fgets is happy to continue reading until newline).

Correct, but using getdelim won't change that, since the required delimiter would still be a newline.

If users feed binary data to this program, the input key passed to the kdf may be substantially shorter than expected.
No idea if there are any practical implications re how this program is intended to be used, though.

The pbkdf2 wrapper (and that program is nothing more) exists to derive a binary key from a non-binary (human memorable) passphrase + binary (hex encoded) salt. If you already had a binary key (i.e. binary input), you wouldn't need to use a kdf.

[joachifm]

Truncation occurs because of strlen(), which getdelim() solves by reporting the number of bytes it read. Presumably, the strength of the derived key would be unaffected by truncation, but it seems recovering the key could accidentally become easier.

As long as users never feed strange inputs to the program, there's no problem, but I think it's unfair to discount the potential (whether or not it's worth doing anything about it).

[MoritzMaxeiner]

Truncation occurs because of strlen(), which getdelim() solves by reporting the number of bytes it read.

Thanks, I overlooked that; this is a good argument for replacing the fgets+strlen pair with getdelim, though as I said, I haven't used this (in a long time) and unless someone else still does you should just drop the whole thing (and keep it from bitrotting further).

Presumably, the strength of the derived key would be unaffected by truncation, but it seems recovering the key could accidentally become easier.

No, because the derived key is used exclusively for accessing an already encrypted LUKS volume.

As long as users never feed strange inputs to the program, there's no problem, but I think it's unfair to discount the potential (whether or not it's worth doing anything about it).

The input is retrieved via explicit shell prompt at boot time and then fed into the program and you can't enter a null byte on the kernel tty (so you wouldn't use one when setting up the LUKS volume, anyway).

[0xABAB]

There are two ways of constructing a software design: One way is to make it so simple that there are obviously no deficiencies, and the other way is to make it so complicated that there are no obvious deficiencies. The first method is far more difficult.

There is a reason I remembered this quote when I read your message.

[MoritzMaxeiner]

There is a reason I remembered this quote when I read your message.

That's your prerogative, but irrelevant to the issue at hand.

[0xABAB]

@Calrama What is the issue at hand according to you?

I can imagine that technically and even mathematically there is no problem, but the thing is, I don't even want to read an explanation of an analysis written by in this case the original author. Not because I don't believe it to be correct, but it's because I don't want someone else to also have to understand that analysis in order to believe the code works. It's simply more costly from a maintenance point of view to require such reasoning.

Consider that someone wants to call this code from another context and then the whole analysis needs to be done again (and typically is never done and it results in a bug). Also note that you haven't even documented a valid input specification.

Really, your code is just a disaster waiting to happen and you apparently need to have this explained multiple times before you get it, and perhaps you will never get it.

There are more issues with your code, btw. For example, you make implementation choices when applying an algorithm that are not referenced to scientific literature or with any other kind of justification.

In short, I would consider this code written by someone who can instruct a computer what to do, but just lacks the wisdom to tell the computer the right things to do.

[MoritzMaxeiner]

@Calrama What is the issue at hand according to you?

The issue is that (as written above) the fgets+strlen pair should be replaced by getdelim iff. it's worth for this project to keep the functionality provided by the semantic block it belongs to. That's not for me to decide, since I don't use it anymore.

I can imagine that technically and even mathematically there is no problem, but the thing is, I don't even want to read an explanation [...]

Again, your prerogative, but not relevant to the issue at hand.

Consider that someone wants to call this code from another context and then the whole analysis needs to be done again

That is always the case if one reuses a piece of code in another context.
This is not new.

(and typically is never done and it results in a bug).

That's a mistake of the person reusing C code for another purpose without doing the proper analysis.

Also note that you haven't even documented a valid input specification.

Of course not, it's an internal component, not part of a public interface.

Really, your code is just a disaster waiting to happen and you apparently need to have this explained multiple times before you get it, and perhaps you will never get it.

The code will result in disaster when someone abuses an internal component for a different purpose without analysing whether its applicable.
Your continued ad hominem attacks (after providing an incomplete bug report) remain irrelevant.

There are more issues with your code, btw. For example, you make implementation choices when applying an algorithm that are not referenced to scientific literature or with any other kind of justification.

That would only be an issue if the derived key was used for something other than decryption of preexisting encryption.

In short, I would consider this code written by someone who can instruct a computer what to do, but just lacks the wisdom to tell the computer the right things to do.

As I have pointed out before, your personal attacks serve no purpose with regards to this issue.

[0xABAB]

@Calrama The hole you been digging for yourself is now so deep that all I have to do now is to give you a little push.

Did you know your C program has undefined behavior?

[MoritzMaxeiner]

@Calrama The hole you been digging for yourself is now so deep that all I have to do now is to give you a little push.

Since after a civil reminder you insist on keeping up the hostile and - frankly - abusive behaviour, I won't spend more of my time on you (feel free to interpret that as you wish, of course, I don't mind) :)

[0xABAB]

@Calrama You are the one who kept insisting that the code was good and that only if others would make a mistake (implying that you are the god of programming) that there would be an issue.

Then finally, when I have carefully positioned you such that there is no escape left, you start to cry like a little baby about how hostile I am?

You were acting as the most obnoxious person I have ever seen here and you needed to be taught a lesson in humility.

The trait most people like you have is that they are unaware of their own limitations and they just keep on sharing their incompetence with the rest of the world.

It is pathetic.

[edolstra]

@0xABAB I've blocked you from the NixOS organization. This isn't the first time you've engaged in personal attacks on contributors, and I've had enough.

[Profpatsch]

@Calrama Don’t let that behaviour get to you; the person suddenly appeared a few days ago (see their profile) and started commenting hundreds of issues/PRs, always with a doubtful attitude. I wasn’t sure what to make of it because it seemed like they were contributing, but apparently they just wanted to burn stuff.

Nothing gained, nothing lost I’d say.

[domenkozar]

I approve @edolstra actions, FYI.

@lukego you're very welcome to reopen the PR, I'll review it shortly.