New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue on truncated certdata.txt file that happens sometimes #1577

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@CaViCcHi
Contributor

CaViCcHi commented Jun 16, 2017

it is possible that sometimes the certdata.txt may not be downloaded correctly, unfortunately there is no checksum available on the file as it is considered a "will always work 100%" but it's happened that the certdata.txt file was downloaded only partially and mk-ca-bundle.pl accepted the file (since it makes no check as of now) and the resulting .crt did not contain all certificates expected

"Added parameter -e to verify that the download of the certdata.txt actually completed succesfully as it has happened that the file was truncated on download and did not contain all certificates expected"

Added parameter -e to verify that the download of the certdata.txt ac…
…tually completed succesfully as it has happened that the file was truncated on download and did not contain all certificates expected
@mention-bot

This comment has been minimized.

mention-bot commented Jun 16, 2017

@CaViCcHi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @watson81, @gknauf and @bagder to be potential reviewers.

@coveralls

This comment has been minimized.

coveralls commented Jun 16, 2017

Coverage Status

Coverage increased (+0.03%) to 73.198% when pulling a7a0814 on CaViCcHi:feature/verify_certdatatxt into 615326f on curl:master.

@bagder

bagder approved these changes Jun 16, 2017

Cool. I approve of these changes, even if the directory check is highly dependent on the serving software providing that directory listing in that format, which makes this feature risking to break in a future when that change. Still I think it can serve a use until then.

But please also update docs/mk-ca-bundle.1 to describe the new option!

@bagder

This comment has been minimized.

Member

bagder commented Jun 16, 2017

I think a better fix, possibly in addition to this, is to check that curl doesn't exit with a non-zero return code. This site provides Content-Length for the data so curl will return an error if it doesn't actually download exactly that amount, which should be sufficient for you...

Your size check is also probably better done as a -I on the actual file URL and not on the directory, as that will make it more robust and likely to be somewhat future-proof.

@jay

This comment has been minimized.

Member

jay commented Jun 16, 2017

I disagree. This to me is a bug. The server provides a content length. If there's a truncation attack CURLE_RECV_ERROR or CURLE_PARTIAL_FILE would happen. It is our mistake for not catching it. I appreciate the contribution but I'd rather we just fix the bug, I can't see any reason to allow a partial certdata.

diff --git a/lib/mk-ca-bundle.pl b/lib/mk-ca-bundle.pl
index 9574f1d..cc36c76 100755
--- a/lib/mk-ca-bundle.pl
+++ b/lib/mk-ca-bundle.pl
@@ -310,7 +310,7 @@ if(!$opt_n) {
         my $proto = !$opt_k ? "--proto =https" : "";
         my $quiet = $opt_q ? "-s" : "";
         my @out = `curl -w %{response_code} $proto $quiet -o "$txt" "$url"`;
-        if(@out && $out[0] == 200) {
+        if(!$? && @out && $out[0] == 200) {
           $fetched = 1;
           report "Downloaded $txt";
         }
@bagder

I'm changing my view on this. We should primarily check the return code .

jay added a commit that referenced this pull request Jun 16, 2017

mk-ca-bundle.pl: Check curl's exit code after certdata download
- No longer allow partial downloads of certdata.

Prior to this change partial downloads were (erroneously?) allowed since
only the server code was checked to be 200.

Bug: #1577
Reported-by: Matteo B.
@jay

This comment has been minimized.

Member

jay commented Jun 16, 2017

I'm changing my view on this. We should primarily check the return code .

curl return code check landed in ec92afc. Thanks for the report @CaViCcHi.

@bagder

This comment has been minimized.

Member

bagder commented Jun 16, 2017

thanks @jay!

@CaViCcHi, with this fix landed, is there actually any issue remaining for you?

@CaViCcHi

This comment has been minimized.

Contributor

CaViCcHi commented Jun 16, 2017

No guys, thanks; I think you know better how the innerworks of curl operate :) thanks @jay && @bagder

@CaViCcHi CaViCcHi closed this Jun 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment