Bug #4962
closedndncert-client: insufficient input validation
100%
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)
Updated by Davide Pesavento over 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.
Updated by Davide Pesavento over 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
Updated by Davide Pesavento over 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.
Updated by Davide Pesavento over 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
Updated by Yufeng Zhang over 5 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.
Updated by Zhiyi Zhang over 5 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.
Updated by Davide Pesavento about 5 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 :
[...]
Updated by Davide Pesavento about 5 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)
Updated by Davide Pesavento about 5 years ago
- Subject changed from ndncert-client: poor input validation to ndncert-client: insufficient input validation
Updated by Davide Pesavento about 5 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.
Updated by Davide Pesavento about 5 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
Updated by Zhiyi Zhang over 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.
Updated by Zhiyi Zhang over 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.
Updated by Davide Pesavento over 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.
Updated by Davide Pesavento about 3 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.