Created attachment 950245 [details] a patch on upstream rpmlint Description of problem: In Fedora 21 crypto policies was introduced [0]. The attached patch allows to warn the maintainer of potential actions he needs to do. That is in accordance to recommendation from FPC: https://fedorahosted.org/fpc/ticket/452 The temporary descriptive URL is: https://fedoraproject.org/wiki/User:Nmav/CryptoPolicies [0]. http://fedoraproject.org/wiki/Changes/CryptoPolicy
Created attachment 951768 [details] A complete patch for fedora package The new patch applies to the fedora git repository for rpmlint.
Ville, what do you think about this change? I don't really want to be carrying a Fedora-only patch if I can avoid it.
Suggestions how to make the patch non-Fedora-only are welcome. The policy it enforces is Fedora only.
Could you elaborate which part specifically of this is Fedora only? At least there is a reference Fedora docs in the description, that should probably be dropped. Does for example the crypto_param_regex need to be made configurable? BTW I'm not a big fan of the additional "strings" call. Would it be possible to extract this information from output of some of the existing calls? Or turn it the other way around so that if crypto calls are found, *then* check for crypto params instead of doing it eagerly and doing nothing with it in the vast majority of cases.
(In reply to Ville Skyttä from comment #4) > Could you elaborate which part specifically of this is Fedora only? At least > there is a reference Fedora docs in the description, that should probably be > dropped. Does for example the crypto_param_regex need to be made > configurable? I'm not sure I understand the question. The policy this patch implements is fedora-specific, thus the whole patch is fedora specific. > BTW I'm not a big fan of the additional "strings" call. Would it be possible > to extract this information from output of some of the existing calls? Or > turn it the other way around so that if crypto calls are found, *then* check > for crypto params instead of doing it eagerly and doing nothing with it in > the vast majority of cases. I'm also not a fan of it, but as far as I understand it is mandated by the current design which splits the BinaryInfo from the BinaryCheck. If you have some proposal on how to achieve the functionality you propose I'd be very interested.
(In reply to Nikos Mavrogiannopoulos from comment #5) > I'm not sure I understand the question. The policy this patch implements is > fedora-specific, thus the whole patch is fedora specific. What exactly about the policy is Fedora specific? For example are the PROFILE=SYSTEM and @SYSTEM names Fedora specific or available generally in openssl/gnutls setups on non-Fedora as well? One thought off the cuff is: make the crypto param names configurable (default to empty list) and construct crypto_param_regex from them; if the list is empty then the whole check is disabled, otherwise it gets run. (Will postpone looking into the strings invocation stuff more closely, will revisit if we can solve the above general hurdle first wrt upstreaming.)
(In reply to Ville Skyttä from comment #6) > (In reply to Nikos Mavrogiannopoulos from comment #5) > > I'm not sure I understand the question. The policy this patch implements is > > fedora-specific, thus the whole patch is fedora specific. > > What exactly about the policy is Fedora specific? For example are the > PROFILE=SYSTEM and @SYSTEM names Fedora specific or available generally in > openssl/gnutls setups on non-Fedora as well? They are fedora-specific.
In that case I'd say the "off the cuff" thought from comment 6 applies -- if you want to upstream this, create an upstream patch along those lines: config value for crypto params as list or a ready made regex (see "config" distributed with rpmlint for examples) and leave the default value as None/empty list (--> empty disables this check) and remove the Fedora reference from the message description, then configure PROFILE=SYSTEM and @SYSTEM crypto params in the Fedora rpmlint package's config.
(In reply to Ville Skyttä from comment #8) > In that case I'd say the "off the cuff" thought from comment 6 applies -- if > you want to upstream this, create an upstream patch along those lines: > config value for crypto params as list or a ready made regex (see "config" > distributed with rpmlint for examples) and leave the default value as > None/empty list (--> empty disables this check) and remove the Fedora > reference from the message description, then configure PROFILE=SYSTEM and > @SYSTEM crypto params in the Fedora rpmlint package's config. I believe I'm misunderstood. I don't care if that is going upstream. I was asked by the FPC to make an rpmlint check for fedora, and that's what I attach here. If you want to send it upstream I'd be glad to help, but I have no time to spend discussing the issue with upstream and how they'd like it to be included.
I am upstream and my opinion was asked in comment 2. I hope I've made it clear now.
(In reply to Ville Skyttä from comment #10) > I am upstream and my opinion was asked in comment 2. I hope I've made it > clear now. Ok that makes that clear, sorry. I'll add the change to my todo list.
Created attachment 960621 [details] generalized previous patch This patch generalizes the previous one. The previous behaviour can be restored by a configuration file containing the following: setOption("WarnOnFunctions", ['SSL_CTX_set_cipher_list','gnutls_priority_set_direct','gnutls_priority_init']) setOption("WarnOnFunctionsExceptIfStringIsPresent", '(^PROFILE=SYSTEM$|^@SYSTEM$)') setOption("WarnOnFunctionsInfo", 'bin-may-not-adhere-to-system-wide-crypto-policy') addDetails( 'bin-may-not-adhere-to-system-wide-crypto-policy', '''This application package calls a function to explicitly set crypto ciphers or SSL/TLS. That may not use the default crypto policy and should be modified in accordance to: https://fedoraproject.org/wiki/Packaging:CryptoPolicies ''')
Tom, can we move forward to having that change in F21 on way or another? It is now an official policy and would be best if rpmlint would help enforce the policies.
WarnOnFunctions and *ExceptIfStringIsPresent should be grouped so that an exception maps to a specific set of functions and not to everything. For example in addition to stuff in the patch's sample config one might want to flag calls for foo() no matter what strings are present and the crypto ones shouldn't affect it. Also, the message identifiers and their details should be specific for a set of functions, not to everything. For example WarnOnFunction could be a modifiable sequence of function-sequence, exception-filter, message-id, detail sequences (empty by default) that could be appended to in configs like this: getOption("WarnOnFunctions").append( (('SSL_CTX_set_cipher_list', 'gnutls_priority_set_direct', 'gnutls_priority_init'), '(^PROFILE=SYSTEM$|^@SYSTEM$)', 'bin-may-not-adhere-to-system-wide-crypto-policy', '...detail for the crypto policy message goes here...')) getOption("WarnOnFunctions").append( (('foo'), None, 'bin-calls-foo', 'Calling foo is a no-no!'))
Ville's suggestions seem on target. I want to get the framework for this change into upstream rpmlint, and not have to carry it as a patch in Fedora forever. Having a custom Fedora config to throw errors/warnings on this condition is fine, but a custom patch that upstream will not take is not.
(In reply to Ville Skyttä from comment #14) > WarnOnFunctions and *ExceptIfStringIsPresent should be grouped so that an > exception maps to a specific set of functions and not to everything. For > example in addition to stuff in the patch's sample config one might want to > flag calls for foo() no matter what strings are present and the crypto ones > shouldn't affect it. Also, the message identifiers and their details should > be specific for a set of functions, not to everything. For example > WarnOnFunction could be a modifiable sequence of function-sequence, > exception-filter, message-id, detail sequences (empty by default) that could > be appended to in configs like this: Sorry but I have no idea how to do that. If you have some example code for that I could work on it.
(In reply to Tom "spot" Callaway from comment #15) > Ville's suggestions seem on target. I want to get the framework for this > change into upstream rpmlint, and not have to carry it as a patch in Fedora > forever. Having a custom Fedora config to throw errors/warnings on this > condition is fine, but a custom patch that upstream will not take is not. I don't disagree, but I don't have the python knowlege to achieve that. If you or any other maintainer is interested to complete that, please step up.
(Aside: You probably also want to check for glib applications that set the environment variable G_TLS_GNUTLS_PRIORITY.)
Created attachment 1057800 [details] generalised version of C call blacklisting Version of the patch that follows suggestions from Comment #4 and Comment #14 (although I've used a dictionary, not a tuple or list)
Created attachment 1057802 [details] non-squashed version of attachment 1057800 [details] this is just to explain my reasoning between particular changes in the patch
Ville, could you take a look at attachment 1057800 [details]?
Now that rpmlint moved to github, and after Hubert asked me to review the patch, wouldn't it be easier to have it in a PR there ? Also, line 158/159, it would be more pythonic to use iteritems I am also not sure that the try/except need to be so large on line 186 ( since that's just for 1 call ), and I think it would be better to just verify if waiver_regex exist before search rather than a try/except.
(In reply to Michael Scherer from comment #23) > Now that rpmlint moved to github, and after Hubert asked me to review the > patch, wouldn't it be easier to have it in a PR there ? > > Also, line 158/159, it would be more pythonic to use iteritems will do > I am also not sure that the try/except need to be so large on line 186 ( > since that's just for 1 call ), and I think it would be better to just > verify if waiver_regex exist before search rather than a try/except. I've put try/except as "ask forgiveness not permission" is more pythonic, but I have no strong feelings either way. The try is so large because it makes it a bit more readable than putting the rest of lines into "else" clause of "try", IMHO
PR with changes from comment #23 created here: https://github.com/rpm-software-management/rpmlint/pull/48 this makes the attachment 1057800 [details] and attachment 1057802 [details] obsolete
Tom can we schedule this for F24 irrespective of upstream? It is taking too long, and our policies depend on that change. In any case that would be a good test for the upstream functionality.
Please continue the upstream patch review/discussion @github, I've just posted some comments there.
Damn Bugzilla. Resetting spot's needinfo for comment 26.
Given that upstream is active in this process and interested in this change, I'd really rather wait for them to merge it. I don't mind carrying a patch _after_ they merge it instead of waiting for a new release.
https://github.com/rpm-software-management/rpmlint/pull/48 was merged Tom, can you backport it?
(In reply to Hubert Kario from comment #30) > Tom, can you backport it? No need, I just tagged release 1.8.
rpmlint-1.8-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-6c7d8111f9
rpmlint-1.8-1.fc21 has been submitted as an update to Fedora 21. https://bodhi.fedoraproject.org/updates/FEDORA-2015-8fa7f53497
rpmlint-1.8-1.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with $ su -c 'dnf --enablerepo=updates-testing update rpmlint' You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-6c7d8111f9
rpmlint-1.8-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with $ su -c 'dnf --enablerepo=updates-testing update rpmlint' You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-8c4e0f236f
rpmlint-1.8-1.fc21 has been pushed to the Fedora 21 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with $ su -c 'dnf --enablerepo=updates-testing update rpmlint' You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-8fa7f53497
rpmlint-1.8-2.fc21 has been submitted as an update to Fedora 21. https://bodhi.fedoraproject.org/updates/FEDORA-2015-8fa7f53497
rpmlint-1.8-2.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-b00be42cfd
rpmlint-1.8-2.fc21 has been pushed to the Fedora 21 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with $ su -c 'dnf --enablerepo=updates-testing update rpmlint' You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-8fa7f53497
rpmlint-1.8-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
rpmlint-1.8-2.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with $ su -c 'dnf --enablerepo=updates-testing update rpmlint' You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-b00be42cfd
rpmlint-1.8-2.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.
rpmlint-1.8-2.fc21 has been pushed to the Fedora 21 stable repository. If problems still persist, please make note of it in this bug report.