besser82 / libxcrypt

Extended crypt library for descrypt, md5crypt, bcrypt, and others
GNU Lesser General Public License v2.1
178 stars 49 forks source link

Stricter checking of invalid salt characters. #138

Closed besser82 closed 2 years ago

besser82 commented 2 years ago

This PR is a proposal for implementing the restrictions as discussed in #135.


For now we consider the following characters to be valid for any salt string in the generic check:

0 1 2 3 4 5 6 7 8 9
a b c d e f g h i j k l m n o p q r s t u v w x y z
A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
. / + - _ = , $ " # % & ' ( ) < > ? @ [ ] ^ ` { | } ~
codecov[bot] commented 2 years ago

Codecov Report

Merging #138 (2a1697c) into develop (1d1c34e) will increase coverage by 0.02%. The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #138      +/-   ##
===========================================
+ Coverage    88.55%   88.57%   +0.02%     
===========================================
  Files           32       32              
  Lines         3607     3615       +8     
  Branches       684      687       +3     
===========================================
+ Hits          3194     3202       +8     
  Misses         228      228              
  Partials       185      185              
Impacted Files Coverage Δ
lib/crypt.c 90.64% <100.00%> (+0.57%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 1d1c34e...2a1697c. Read the comment docs.

besser82 commented 2 years ago

@solardiz Should be fine now.

besser82 commented 2 years ago

@zackw You can ignore the one failing test, as it failed for $foo and not the code changes.

besser82 commented 2 years ago

Looks OK to me now.

Thank you for your time and review!

besser82 commented 2 years ago

@zackw Do you have any further remarks or objections on this?

zackw commented 2 years ago

I’m unfortunately not going to be able to look at this until tomorrow.

On Thu, Aug 5, 2021, at 5:48 PM, Björn Esser wrote:

@zackw https://github.com/zackw Do you have any further remarks or objections on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/besser82/libxcrypt/pull/138#issuecomment-893830162, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACPSC3HFE3GEUFAZAHCLQDT3MBLDANCNFSM5BTYK65A. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email.

besser82 commented 2 years ago

Fixed. Let's wait for the CI and I'll merge on cli then.