Project

General

Profile

Actions

Bug #4962

closed

ndncert-client: insufficient input validation

Added by Davide Pesavento almost 5 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Start date:
Due date:
% Done:

100%

Estimated time:

Description

I can crash ndncert-client interactive procedure with a large variety of inputs. This is not very user friendly.

Some inputs I found so far (note that the crash reasons are all different from each other)

***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
1
Segmentation fault (core dumped)
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
-1
terminate called after throwing an instance of 'std::length_error'
  what():  basic_string::_M_create
Aborted (core dumped)
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
foo
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoi
Aborted (core dumped)
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
NONE
Step 1: Please type in the CA Name
Got NACK
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
0
Step 1: Please provide information for name assignment
Please provide the argument: email : 
foo@bar.com
Got it. This is what you've provided:
email : foo@bar.com
If everything is right, please type in OK; otherwise, type in REDO
foo
Got NACK

(I'm not sure about the last two, could be a different bug)

Actions #1

Updated by Davide Pesavento almost 5 years ago

In the last case from the issue description, the "got NACK" part is due to a missing route on the testbed (unrelated problem), but there is still a bug because every user input except "REDO" is interpreted as "OK" and the procedure continues.

Actions #2

Updated by Davide Pesavento almost 5 years ago

This is also very unfriendly to the user (note that I typed "email" all lowercase):

Step 3: Please type in the challenge ID from the following challenges
    PIN
    Email
email
Cannot recognize the specified challenge. Exit
Actions #3

Updated by Davide Pesavento almost 5 years ago

What's worse is that during all these failed tries, ndncert did create a few (self-signed) certificates in my local keychain, a new one each time, but they were never removed when the procedure failed or was interrupted. When I finally managed to get it working and obtain a certificate from the CA, ndnsec list was showing a bunch of certificates with very similar names and no indication about which one was the "right" one.

So, at the very least, ndncert-client should: 1) delete the temporary self-signed certs if the procedure fails or is aborted; 2) print the name of the issued cert at the end of the procedure.

Actions #4

Updated by Davide Pesavento almost 5 years ago

The user input to the "Please type in your expected validity period..." prompt is not validated either. I entered a negative number and the tool happily accepted it.
Interestingly, the CA was also perfectly happy to give me a certificate with a negative validity period!

       +->  /ndn/edu/ucla-ndncert/3403139456692156406/KEY/%D7%BCH%1F%BF%92-%94/NDNCERT/5297390542062471952
            Certificate name:
              /ndn/edu/ucla-ndncert/3403139456692156406/KEY/%D7%BCH%1F%BF%92-%94/NDNCERT/5297390542062471952
            Validity:
              NotBefore: 20190627T235508
              NotAfter: 20190627T225441
            Public key bits:
              MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEa0I+e916g4ebABXBplUpX2KYhYpW
              qGXi9MEUeuXD9nQ8LoU4J6O12UTakpblD/DRKm/4KNlMw9DhwAfyA+K/PQ==
            Signature Information:
              Signature Type: SignatureSha256WithRsa
              Key Locator: Name=/ndn/edu/ucla-ndncert/KEY/%82%F4%E94R%0B%26%18
Actions #5

Updated by Yufeng Zhang over 4 years ago

Davide Pesavento wrote:

I can crash ndncert-client interactive procedure with a large variety of inputs. This is not very user friendly.

Some inputs I found so far (note that the crash reasons are all different from each other)

***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
1
Segmentation fault (core dumped)
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
-1
terminate called after throwing an instance of 'std::length_error'
  what():  basic_string::_M_create
Aborted (core dumped)
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
foo
terminate called after throwing an instance of 'std::invalid_argument'
  what():  stoi
Aborted (core dumped)
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
NONE
Step 1: Please type in the CA Name
Got NACK
***************************************
Index: 0
CA prefix:/ndn/edu/ucla-ndncert
Introduction: NDN Testbed CA
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
0
Step 1: Please provide information for name assignment
Please provide the argument: email : 
foo@bar.com
Got it. This is what you've provided:
email : foo@bar.com
If everything is right, please type in OK; otherwise, type in REDO
foo
Got NACK

(I'm not sure about the last two, could be a different bug)

Thank you, Davide. I've fixed most of the bugs you mentioned in this issue. However, I am not sure about the length validation part. I need to ask Zhiyi where the maximum time limit is stored in the configuration. I'll push it to Gerrit soon.

Actions #6

Updated by Zhiyi Zhang over 4 years ago

The CA configuration file contains the maximum validity period while the client side does not know.
I will add the minus value check to both the CA and the client command-line tool.

Actions #7

Updated by Zhiyi Zhang over 4 years ago

  • Status changed from New to In Progress
Actions #8

Updated by Davide Pesavento over 4 years ago

  • % Done changed from 0 to 50

As of ndncert commit 55d9860fc346d04f4c844e80304dee7cb47de6da, most of the above problems are fixed, except the following one:

***************************************
Index: 0
CA prefix:/ndn/edu/ucla/yufeng
Introduction: UCLA CA in NDN Testbed
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
NONE
Step 1: Please type in the CA Name
Got NACK

I discovered another problem though: if I press Ctrl+D (EOF) at the email, validity period, challenge type, or any of the confirmation prompts, the program enters an infinite loop. Example below:

Please provide the argument: email : 
Got it. This is what you've provided:
email : 
If everything is correct, please type in OK; otherwise, type in REDO
Please provide the argument: email : 
Got it. This is what you've provided:
email : 
If everything is correct, please type in OK; otherwise, type in REDO
Please provide the argument: email : 
[...]
Actions #9

Updated by Davide Pesavento over 4 years ago

Another bug:

***************************************
Index: 0
CA prefix:/ndn/edu/ucla/yufeng
Introduction: UCLA CA in NDN Testbed
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
0
Step 1: Please provide information for name assignment
Please provide the argument: email : 
foo
Got it. This is what you've provided:
email : foo
If everything is correct, please type in OK; otherwise, type in REDO
ok
Step 2: Please type in your expected validity period of your certificate. Type the number of hours (168 for week, 730 for month, 8760 for year). The CA may change the validity period if your expected period is too long.
1
The validity period of your certificate will be: 1 hours
Step 3: Please type in the challenge index that you want to perform
    0 : email
    1 : pin
0
The challenge has been selected: email
Step 4: Please satisfy following instruction(s)
Please_input_your_email_address
Please provide the argument: email : 
foo
Got it. This is what you've provided:
email : foo
If everything is correct, please type in OK; otherwise, type in REDO
ok
terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::property_tree::ptree_bad_path> >'
  what():  No such node (selected-challenge)
Aborted (core dumped)
Actions #10

Updated by Davide Pesavento over 4 years ago

  • Subject changed from ndncert-client: poor input validation to ndncert-client: insufficient input validation
Actions #11

Updated by Davide Pesavento over 4 years ago

Davide Pesavento wrote:

What's worse is that during all these failed tries, ndncert did create a few (self-signed) certificates in my local keychain, a new one each time, but they were never removed when the procedure failed or was interrupted. When I finally managed to get it working and obtain a certificate from the CA, ndnsec list was showing a bunch of certificates with very similar names and no indication about which one was the "right" one.

So, at the very least, ndncert-client should: 1) delete the temporary self-signed certs if the procedure fails or is aborted; 2) print the name of the issued cert at the end of the procedure.

This is still a problem in the current code too. Implementing (2) would be really trivial and could help the user a lot already. (1) is a bit more involved, I could open a separate issue for that one.

Also, when the certificate is issued successfully and installed locally, the self-signed cert is still set as the "default" cert for the key. Maybe this logic could be smarter and set the ndncert-issued cert as default if there are no other pre-existing certificates set as default.

Actions #12

Updated by Davide Pesavento over 4 years ago

Another one...

***************************************
Index: 0
CA prefix:/ndn/edu/ucla/yufeng
Introduction: UCLA CA in NDN Testbed
***************************************
Step 0: Please type in the CA INDEX that you want to apply or type in NONE if your expected CA is not in the list
0
Step 1: Please provide information for name assignment
Please provide the argument: email : 
foobar
Got it. This is what you've provided:
email : foobar
If everything is correct, please type in OK; otherwise, type in REDO
ok
Step 2: Please type in your expected validity period of your certificate. Type the number of hours (168 for week, 730 for month, 8760 for year). The CA may change the validity period if your expected period is too long.
123456789
The validity period of your certificate will be: 123456789 hours
Interest sent time out
Actions #13

Updated by Davide Pesavento over 4 years ago

Any progress here?

Actions #14

Updated by Davide Pesavento about 4 years ago

Ping?

Actions #15

Updated by Zhiyi Zhang about 4 years ago

Davide Pesavento wrote:

Any progress here?

I think this is caused because the CA on spurs is down while the new CA has not started.

Actions #16

Updated by Zhiyi Zhang about 4 years ago

  • Status changed from In Progress to Code review

Davide Pesavento wrote:

Any progress here?

I think this is caused because the CA on spurs is down while the new CA has not started.

Actions #17

Updated by Davide Pesavento about 4 years ago

What are you talking about? I reported several problems above. Which one is caused by the CA being down? Also note that that happened 5 months ago, I don't think the CA was down at that time.

Actions #18

Updated by Davide Pesavento over 2 years ago

  • Status changed from Code review to Closed
  • % Done changed from 50 to 100

Siqi reports that, with the major rewrite for v0.3, the above bugs are no longer reproducible.

Actions

Also available in: Atom PDF