Make sure dearmor correctly handles large lines

[ni4]

Description

Construct corresponding data files and make sure rnp correctly handles following cases:

• too many spaces in front of armored message (>32k)
• too large header name/values
• too large single armored line. Too large is about 32-64k, i.e. larger internal buffers size.

Separate test file should be added to rnp_tests suite.

[ni4]

@dfwdeveloper would you like to take this issue? Cannot assign it to you until commented.

[ronaldtse]

@dfwdeveloper says he's OK with this task but will wait for his reply here.

[dfwdeveloper]

@ni4 Yes, this is a good first start.

[ronaldtse]

Thanks @dfwdeveloper !

[ronaldtse]

This is from @dfwdeveloper off channel:

I have my RHEL up and running. Had a lot of trouble getting all the packages working. Currently, am using the binary rnp version 0.12. The build requires a lot of conflicting packages, slowing things down.

Per the trial task, the

too many spaces in front of armored message (>32k)

• Could the “armored message” be specifically explained? Is this the .asc file?

too large header name/values

• Also, which headers name/values? More explanation would be great.

too large single armored line.

• Which armored line? Any better explanation of what is the “armored line?”

Too large is about 32-64k, i.e. larger internal buffers size.

@ni4 could you help let him know? Thanks.

[ni4]

@ronaldtse @dfwdeveloper sure:

Could the “armored message” be specifically explained? Is this the .asc file?

I should add link to the RFC at the beginning, sorry. Please see the section 6.2 of the OpenPGP specification: https://gitlab.com/openpgp-wg/rfc4880bis/blob/master/draft-ietf-openpgp-rfc4880bis-08.txt

Also, which headers name/values? More explanation would be great.

The ones described in RFC (like Hash, Version, Comment, etc) as well as just ones with dummy names.

Which armored line? Any better explanation of what is the “armored line?”

Line of Base64-encoded data without breaks/EOLs.

[ni4]

@dfwdeveloper I looked at your changes, and those looks good except some comments (and missing long armored lines test). Could you please create a PR here from your fork so we could discuss changes in more convenient way? Thanks!

[dfwdeveloper]

A PR has been created.

Does the "Comments" line count as a header/values plus a long armored lines test? Will add some comments.

[ni4]

@dfwdeveloper Thanks. I added comments there, so let's continue further communication in the PR #947

[ni4]

@antonsviridenko Would you like to take this task and finish the PR #947 so we have less mess?

[antonsviridenko]

ok

[ni4]

Thanks!

[antonsviridenko]

@ni4 maybe it would be better to make a new PR from scratch? :P Looks like data files do not address test cases and for unknown reasons only one API function is called in tests - rnp_detect_key_format().

[ni4]

@antonsviridenko Sure, feel free to create a new PR, and close the older one once new is merged.

[antonsviridenko]

too many spaces in front of armored message (>32k)

@ni4 let's clarify this part

I've looked through OpenPGP spec, it does not say if any whitespace characters are allowed in front of armored message at all. Note that all these Armor Header Lines are to consist of a complete line. That is to say, there is always a line ending preceding the starting five dashes, and following the ending five dashes. The header lines, therefore, MUST start at the beginning of a line, and MUST NOT have text other than whitespace following them on the same line. These line endings are considered a part of the Armor Header Line for the purposes of determining the content they delimit. This is particularly important when computing a cleartext signature (see below).

So I guess you did not mean only literal space characters here, but whitespace characters (newlines, spaces, tabs). RNP now does not allow any characters before the armor header line, GnuPG allows up to ~1024 characers before the armored message starts.

[ni4]

@antonsviridenko Yeah, I mean whitespaces, incorrect wording. We may allow 1024 characters as well, but also add tests which make sure that things go well when there are 32K+ characters (no buffers overflowed and so on).

[antonsviridenko]

I've found that one of these test cases is partially addressed in test_ffi_dearmor_edge_cases:

too large header name/values

https://github.com/rnpgp/rnp/blob/master/src/tests/ffi.cpp#L4135

but it tests for >1024, not 64k. Maybe it would be better to extend test_ffi_dearmor_edge_cases with appropriate larger test data files instead of creating new test entry? In order to avoid duplication

[ni4]

Maybe it would be better to extend test_ffi_dearmor_edge_cases with appropriate larger test data files instead of creating new test entry?

Sure, it would be better in this case.