dolphinsmalltalk / Dolphin

Dolphin Smalltalk Core Image
MIT License
301 stars 58 forks source link

DophinSure Example Authority package example not working #1169

Closed rko281 closed 2 years ago

rko281 commented 2 years ago

From comp.lang.smalltalk.dolphin 26/07/2022:

The example give in the DolphinSure Example Authority package no longer works:

MyOwnCertificate serial: 'MY100002' owner: 'My Own Company Inc.' details: 'security@company.com'

The printString for the certificate fails with the error STBFiler - Input stream contains an invalid class (AnsiString). This can be fixed by changing the reference to #String to #AnsiString in the method DolphinSureCertificateInfo class>>binaryReadFrom:context::

binaryReadFrom: aStream context: anObject 
    "Answers an instance of this class from its binary STB representation on aStream. Because STB is inherently an insecure format
    we must use an STBInFiler that especially guarded with a ValidatingClassLocator to that only expected, non-dangerous
    classes can be loaded from the STB stream."

    | inFiler validClasses |
    validClasses := (Set new)
                add: #Date;
                add: #AnsiString;   "<- change here"
                add: #LargeInteger;
                add: #SmallInteger;
                add: self name;
                yourself.
    inFiler := (STBValidatingInFiler on: aStream)
                validationBlock: [:className | validClasses identityIncludes: className];
                context: anObject;
                yourself.
    ^inFiler next

The same change is probably also needed in DolphinSureTrustedData class>>binaryReadFrom:context:.

With this fixed a further error occurs in the example workspace that opens. The line

(DolphinSureCertificateStore myCertificates atSerial: 'MY100002' ifAbsent: []) show.

...fails with an STBFiler error. This originates in DolphinSureCertificateStore>>atSerial:ifAbsent: since the 'bytes' returned from the Registry lookup is actually an Array of Strings of Integers. This can be fixed in a quick-and-dirty way as follows:

atSerial: aStringSerialNo ifAbsent: exceptionHandler
    "Answers the certificate stored in the receiver at aStringSerialNo. Evaluates
    the exceptionHandler if no such certificate is found"

    | bytes |
    bytes := self regKey subValues at: aStringSerialNo ifAbsent: [ ^exceptionHandler value ].
    ^Object fromBinaryStoreBytes: (bytes collect: [ :each | each asNumber]) asByteArray

A similar change is also needed in DolphinSureCertificateStore>>default (although possibly the fix should be in the registry lookup code?).

Finally, even with these fixes applied, the final part of the example fails to validate the test data ('It was not possible to decode some trusted signed data. The data is either corrupt or has been tampered with.'). Possibly the hard-coded private key is invalid following the changes in #537 ?

blairmcg commented 2 years ago

The issues originate from the addition of Unicode support in 7.1. The set of valid classes allowed by the filer needed to be changed when String became abstract, and problem with the registry data was caused by 303e817f93a53f90bc4499a36370910f0fcd53d9, which broke direct storage of ByteArrays as registry values.

These issues have been fixed in Dolphin 8:

  1. The STB validation issues as part of a commit in January 2021 moving all the DolphinSure classes into namespaces (e5aa7faf8002fb7aba4b5d7c17b6ddee6e2b601a). These should be fairly easy to extract, and included a test in DolphinSureTrustedDataTest>>#testBinaryReadOldCertificate.
  2. Registry serialisation in Dolphin 8 is completely overhauled and would be more difficult to backport. We can easily make a more targeted fix, however.

I extracted/created the fixes while investigating, and will send shortly.

Finally, even with these fixes applied, the final part of the example fails to validate the test data ('It was not possible to decode some trusted signed data. The data is either corrupt or has been tampered with.').

If you are referring to the last expression, then that is what the last two lines are trying to demonstrate 😊. The data has been tampered with by the penultimate expression.

rko281 commented 2 years ago

If you are referring to the last expression, then that is what the last two lines are trying to demonstrate 😊. The data has been tampered with by the penultimate expression.

Thanks and apologies!

blairmcg commented 2 years ago

If you are referring to the last expression, then that is what the last two lines are trying to demonstrate 😊. The data has been tampered with by the penultimate expression.

Thanks and apologies!

No need to apologise. It confused me for a while until the penny dropped.

blairmcg commented 2 years ago

This didn't auto-close because the PR was not merged to the default branch, which is now master, although I'm confident the changes aren't required in master anyway as they are either present or superseded. Close it if/when you are happy.

rko281 commented 2 years ago

Tested and working.