Skip to content
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

Remove DigestAlgorithmID DerNull default value #163

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eulercb
Copy link

@eulercb eulercb commented Dec 2, 2018

Fix issue #162.

Below is a sample of a signed CMS before the patch:
MIIEagYJKoZIhvcNAQcCoIIEWzCCBFcCAQExDzANBglghkgBZQMEAgEFADATBgkqhkiG9w0BBwGgBgQEVEVTVKCCAoQwggKAMIIBaKADAgECAggX/rexsv6GJjANBgkqhkiG9w0BAQsFADAAMB4XDTE3MTIwMjE2MjM1NFoXDTE5MTIwMjE2MjM1NFowADCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBALf3OMZ75RFL5zTZNQViSPF+2AuNlW6haqaZFjfIRdlZQ97MoqFTxzPgPBr3sovaraO5/3kolxm03bj2BGNcShp8lDKQKk3An1qKN8no7XLDXV216PukH/lbzLX1E78gKNeEovX/TmtiDTCiJValzmflXZ58HAhaffOaKbC3qdU1BGsuNx+KTIogjjR0ScPqX5WeaGeOc3WHXFxmkoc/CNx0KREMGwAYiaZhjNsEfFcVG9inqS/loVz4+qviXxOwUXFrYiQhuDLQFn9PVAtTiXZ/SFXk+/HDmUXisRdXDZfdKcE4z9X1IrdEl9YZrXgYIlVswmF73EdcC/FHThosnI0CAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAqROROypDe2KKo5QQlHcn1KQ8nkuNZ6w3y8kldf5RXkf6tKjvXlMDoXxFfXL0JEAPrkmPEjC81A6hRpaS3PTEwKqOzBTGE9fB2N2CzX3l5utgvZQ1j+bBlCiyKzYwaR237hA/uprSycKYP5vRwATn6fjBGL4UDvNTauApHt01A3TKk4LdswPgBf7fT1bChOd6Y3RVHJecGDJueFhJAZplUPXH4rr6wv4QayvSztt8H3aY56gbtQ13eqJlZ7jZrrcrUEdghOscF+YwPkboQPKPincygzOukdLjJadbGeWUF8jvHtQwCLTpYx1wRsvBHBxQS6aaxf0q5+C4cSCxCFcO/TGCAaIwggGeAgEBMAwwAAIIF/63sbL+hiYwDQYJYIZIAWUDBAIBBQCgaTAYBgkqhkiG9w0BCQMxCwYJKoZIhvcNAQcBMBwGCSqGSIb3DQEJBTEPFw0xODEyMDIxNjIzNTRaMC8GCSqGSIb3DQEJBDEiBCCU7gWTNeWH5QHMS/kGE+CBTwCnsIvHxkj9hloq9qIswjANBgkqhkiG9w0BAQEFAASCAQCEBggHRHDqOJixEO2PWyml7YlcVRuit6BkAZR0TeTnN8u+kP2Q2as2Gyf5tIqSwgyQuVQznE+9+3SeC2hQUPu9VbIBn0rX9siXbPPqfMt5u8VfNyVennbuqtdW0gBWo8DGVMTCNeZKXmW7fnsYSh2AuMSdKFXmRaj5BJlEFjY69F2Gmc/PxoqH5kf1yuznZ+gC7XrTHuHt3Hqmk+0fvG4DWMqKDqvAyhc+fVb//HOsjAtc6gP8nfaw5/jEeCAc633YOVkv6BTM29D1DSr7+RWHKa3FNFgnKzaMQH873xh+EN8CHe7g4MeQmgWg0+t1g3UtRBCzDOwcPuUZwQc5MWMM

And one after the patch:
MIIEaAYJKoZIhvcNAQcCoIIEWTCCBFUCAQExDTALBglghkgBZQMEAgEwEwYJKoZIhvcNAQcBoAYEBFRFU1SgggKFMIICgTCCAWmgAwIBAgIJALN9yL4UVw5iMA0GCSqGSIb3DQEBCwUAMAAwHhcNMTcxMjAyMTYyNjE4WhcNMTkxMjAyMTYyNjE4WjAAMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA2qk1JDtshgz7r81yHSyvr5dc8oay3FV7WqMzcYlNPsdbgd2hbSlkWrcNnKzyokmqEId0wV2wRQ6Zvaw0v9yFwGSwsOv+mQofc6iOi6vc/VRFib6o06dtnuBZrrpwl6P2dt8uUb3BtwwD+2MD+q3X/LG/gK9RXLZh9kWmGbTXdU8GHSK/iO41pU88ZQ/iuzJrEYF89W/XCNYdKDf20jv8N78AL9BUawAJrqGRgtS+HT9oodLzYGoEIR9UeIao/WxmGncUJuVqZSWhOz9svGyPwHFiKbC2GkUQq2/gnzHN5HTlVHmk8u4RtEnh5USZJ5NRuS5pHk2gD1Mc/GCYjC9qsQIDAQABMA0GCSqGSIb3DQEBCwUAA4IBAQA4rsYYH8JWlHElOK34arfccQXnYRW2dyLBtKMB5rLawV3rJ6v8YwtXPVInKJANCcPMpmzsvNqH7EC2wxePBU/L01O+gxAZbhKDFxBmRkIsa5THf19B8pdUM3NjhapNtLJ5zDMzF9LoRNxWix1CLTGdOzCasGhHuHqw6QlieXf5xUp5Mqxo7QyJAP30O0OdN6+mK8obR8fk4AnzIacx2yyZTox6z9GvhQN1TOz5Q+P45YdEM2Y5jclKhktNvcMxf1ENPjcgmKiv3BrYR7hVk31vzwIL5rf9LTZJSrbyk5in8gQBjmTE0f6rOcHnBpUgRDqpYCmGY4K8WeNZVLnZGoHzMYIBoTCCAZ0CAQEwDTAAAgkAs33IvhRXDmIwCwYJYIZIAWUDBAIBoGkwGAYJKoZIhvcNAQkDMQsGCSqGSIb3DQEHATAcBgkqhkiG9w0BCQUxDxcNMTgxMjAyMTYyNjE5WjAvBgkqhkiG9w0BCQQxIgQglO4FkzXlh+UBzEv5BhPggU8Ap7CLx8ZI/YZaKvaiLMIwDQYJKoZIhvcNAQEBBQAEggEANQ8fYzEvkE7U81swXfZmo+HX9myGjqQlA7bLqTbK4oX+VOAvY7tNgf5MlYdIrRZsPO3i0GZq3x4DVzWzRXM3dt5sj02GbYeJzJcusxX6p22NmA6RfsN7celynhdtnj+CjAec+pYmyZaQokSCtGMSybVg2SnK50XtUJJRmL/6iCQLX8fuyIJpULQovbE9aQ6e+3US9dHqCGitE74VAmLb7R9XAYJoVDvLlasQbZFNZ9h80tMg0YQanq6WqBzheRYUuygCt1RmzXhAFgVFAJwF+cIjsq0fGP1s2vQSDN6i/M2euUiCazicSKRZqnXFC45s09us61IHgHborQ67DLONhQ==

I hope there are no side effects by this change.

@bcgit
Copy link
Collaborator

bcgit commented Dec 2, 2018

https://tools.ietf.org/html/rfc3370 section 2 specifies that the NULL parameters field is mandatory for some algorithms, optional for some others. For compatibility reasons we have always added it.

I do note that RFC 5754 section says that implementations MUST leave out the NULL for SHA-2 except where it is used in PKCS#1... it does say both should be accepted, but we need to look into this further. The patch proposed is not enough but it does appear something should be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants