Debug xml dsig #4

fisx · 2019-02-12T09:19:47Z

These changes make XML signature validation more robust, and also more correct.

Tasks:

capture stdout, stderr noise from libxml2 and feed it into an error that is returned;
make sure that the signed subtrees are self-contained by applying an extra round of canonicalization;
fix test suite (currently: "failed to canonicalize input: xmlReadMemory: illegal operation (Inappropriate ioctl for device)")

ChrisPenner · 2019-02-12T09:58:09Z

SAML2/XML/Signature.hs

+        doc' <- case mdoc' of
+          Right x
+            -> pure x
+          Left (err :: SomeException)


It'd be nice to move the SomeException type annotation up into try @SomeException so the exception type stays close to the try and makes it clear that we plan to catch ALL exceptions 😄

ChrisPenner · 2019-02-12T10:00:45Z

SAML2/XML/Signature.hs

+          Left err -> throwError . SignatureParseError $ show err
+          Right v -> pure v
+
+  signedInfoElem :: BS.ByteString


Some docstring or comments explaining the magic xPaths would be nice 😄

Agreed, but i would have to figure out what that code does first... (It's not mine)

ChrisPenner · 2019-02-12T10:03:50Z

SAML2/XML/Signature.hs

+      $ mapM (`verifyReference` signedSubtree) (signedInfoReference signedInfoTyped)
+
+  unless (all isRight referenceChecks) $
+    throwError . SignatureVerifyBadReferences $ show (signedInfoReference signedInfoTyped, referenceChecks)


If there's a left in the referenceChecks should we possibly sequence it to collect the error out? Or is it more descriptive to print everything out like this?

It's a list of Eithers, and I want to keep track of how many errors there are, and which precisely.

I agree it looks weird, but I can't think of a better way to do it. Wanna try something and add a commit?

In that case; how about show (signedInfoReference signedInfoTyped, lefts referenceChecks) instead? lefts will just grab all the errors from the list of Eithers

ChrisPenner · 2019-02-12T10:08:03Z

SAML2/XML/Signature.hs

+capture' :: String -> IO a -> IO a
+capture' actionName action = hCapture [stdout, stderr] action >>= \case
+  ("", !out) -> pure out
+  (noise, _) -> throwIO . ErrorCall $ actionName <> ": " <> noise


If any noise on stdout indicates an error; it's probably helpful to have the output of stderr appended here too.

i think that's what this does, no?

(thanks for the other comments, more changes coming up!)

Ahh; I see! I misunderstood; It looked at a glance that the tuple matched up with [stdout, stderr]; but in hindsight that does't make sense 👍

ChrisPenner · 2019-02-12T10:18:47Z

It's all greek to me 🤷‍♂️

Did this not change any behaviour in any of the tests? If that's the case we should almost certainly add some new tests to capture whatever was wrong before this change.

fisx · 2019-02-12T10:22:17Z

It's all greek to me

Did this not change any behaviour in any of the tests? If that's the case we should almost certainly add some new tests to capture whatever was wrong before this change.

It does make one test fail. See PR description. :-)

To reproduce, run stack test --fast and follow the error.

fisx · 2019-02-12T10:25:13Z

oh, and:

It's all greek to me

i've stared at this for over a day now, and not for the first time, and i feel the same way. :-). i don't think there is a way to implement xml:dsig though that would make the reader of the code feel any better about the world. that's just how xml standards are.

fisx · 2019-02-12T10:51:36Z

xmlReadMemory: illegal operation (Inappropriate ioctl for device)

i think i know what's going on here.

Since libxml2 doesn't return errors, but spills them on stdout, stderr, I use hCapture that reads those into a string and returns it with together with the value.

hCapture seems to change the type of the file handle (from what to what?). libxml2 not only spills stuff on stderr, but also performs a ioctl operation on stderr (or stdout) that is supported before hCapture, but not after. hence, ENOTTY.

The first solution I can think of is to trap and ignore this error c14n.

I think this test had some issue with whitespace being changed after signing, and canonicalization does not touch whitespace that much. I am removing it because it is very likely that this is the issue, and very hard to robustly confirm it.

fisx · 2019-02-13T11:33:43Z

ready for review!

neongreen · 2019-02-13T11:57:37Z

SAML2/XML.hs

+--
+-- TODO: change this function to return 'HXT.XmlTrees' and replace 'samlToDoc' with it.
+samlToDoc' :: XP.XmlPickler a => a -> HXT.XmlTree
+samlToDoc' = head . getChildren . head


samlToDoc' = head . getChildren . samlToDoc ?

Or do you want it to remain like it is so that it would be easier to get rid of samlToDoc in the future?

yes and yes.

neongreen · 2019-02-13T12:21:50Z

test/XML/Signature.hs

@@ -302,11 +306,6 @@ examples = zipWith ($) xs [1..]
  where xs :: [Int -> VerifyExample]
        xs =
          [ VerifyExample
-            "<ds:KeyInfo xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\"><ds:X509Data><ds:X509Certificate>MIIDpDCCAoygAwIBAgIGAWOMMryDMA0GCSqGSIb3DQEBCwUAMIGSMQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzENMAsGA1UECgwET2t0YTEUMBIGA1UECwwLU1NPUHJvdmlkZXIxEzARBgNVBAMMCmRldi02MDc2NDgxHDAaBgkqhkiG9w0BCQEWDWluZm9Ab2t0YS5jb20wHhcNMTgwNTIzMDg1MTA1WhcNMjgwNTIzMDg1MjA1WjCBkjELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBE9rdGExFDASBgNVBAsMC1NTT1Byb3ZpZGVyMRMwEQYDVQQDDApkZXYtNjA3NjQ4MRwwGgYJKoZIhvcNAQkBFg1pbmZvQG9rdGEuY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA2HkpOuMhVFUCptrVB/Zm36cuFM+YMQjKdtqEoBJDLbtSbb7uFuvm5rMJ+1VSK5GKAM/Bec5WXTE2WMkifK5JaGOLS7q8+pgiWmqKE3KHMUmLAioe/1jzHkCobxis0FIVhyarRY97w0VMbDGzhPiU7pEopYpicJBzRL2UrzR+PebGgllvnaPzlg8ePtr9/xMv0QTJlYEyCctO4vT5Qa5Xlfek3Ox5yMJM1JPXzn7yuJN5R/Nf8jFprsdBSxNMzkcTRFGy8as2GCt/Xh9H+ef4CxSgRK5UXcUCrb5YMnBehEp2YiuWtw8QsGRR8elgnF3Uw9J2xEDkZIhurPy8OYmGNQIDAQABMA0GCSqGSIb3DQEBCwUAA4IBAQA7kxxg2aVjo7Oml83bUWk4UtaQKYMEY74mygG/JV09g1DVMAPAyjaaMFamDSjortKarMQ3ET5tj2DggQBsWQNzsr3iZkmijab8JLwzA2+I1q63S68OaW5uaR5iMR8zZCTh/fWWYqa1AP64XeGHp+RLGfbp/eToNfkQWu7fH2QtDMOeLe5VmIV9pOFHnySszoR/epMd3sdDLVgmz4qbrMTBWD+5rxWdYS2glmRXl7IIQHrdBTRMll7S6ks5prqKFTwfPvZVrTnzD83a39wl2jBJhOQLjmSfSwP9H0YFNb/NRaDbSDS7BPuAlotZsaPZIN95tu+t9wmFwdxcVG/9q/Vu</ds:X509Certificate></ds:X509Data></ds:KeyInfo>"


Why was this removed?

see commit message.

neongreen · 2019-02-13T12:26:30Z

SAML2/XML/LibXML2.hsc

-      xmlReadMemory p (fromIntegral l) nullPtr nullPtr (#{const XML_PARSE_NOENT} .|. #{const XML_PARSE_DTDLOAD} .|. #{const XML_PARSE_DTDATTR} .|. #{const XML_PARSE_NONET} .|. #{const XML_PARSE_COMPACT})
-  newDoc d
+  resetErrno
+  let config :: CInt


Would be nice to have comments for those flags, because it's not obvious from the names what they do.

it's not my code, and i only touched these to make it slightly easier to read. also, i would argue this information belongs into the libxml2 manual.

SAML2/XML/Signature.hs

neongreen · 2019-02-13T12:35:16Z

I can't really vouch for the logic here. Having more tests for happy and unhappy paths would be nice: good signature, good signature but non-canonical encoding, bad signature, missing signature.

Co-Authored-By: fisx <[email protected]>

fisx · 2019-02-13T20:01:49Z

I can't really vouch for the logic here. Having more tests for happy and unhappy paths would be nice: good signature, good signature but non-canonical encoding, bad signature, missing signature.

i agree, but i won't do it in this PR. note that some of the tests you suggest are in saml2-web-sso (or even in wire-server, i would have to read up on the details).

i will merge this now, if any of the answers i give are not enough please yell and i'll handle that in separate PRs.

neongreen · 2019-02-13T20:05:39Z

Okay

…

On Wed, Feb 13, 2019, 21:04 fisx ***@***.*** wrote: Merged #4 <#4> into master. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABc-arhE4SQT0p5wBW5i5ToCGnby0oWHks5vNG_FgaJpZM4a18E7> .

fisx added 9 commits February 11, 2019 14:42

Cleanup

cff904c

Fix: remove impossible error case.

8ab9731

Cleanup

346f6cb

Cleanup

9c557ad

Fix: do not lose namespace declarations when cutting out signed subtree.

64b0562

Fix: capture stdout, stderr and translate into locatable error.

7766955

Cleanup

d8159e5

Fixup 64b0562

905cac0

Haddocks

6a1ddc7

ChrisPenner reviewed Feb 12, 2019

View reviewed changes

Review

063300a

Review

11eae9c

fisx added 9 commits February 12, 2019 14:00

Cleanup (polish error messages some more)

dfc8f7f

Cleanup

da21a2b

Fix: a better approach to getting a signed subtree.

0e268ee

Fix: how xpath appearently works.

eb0bccf

Make libxml2 parser wrapper handle stderr noise and errors better.

50f3b38

Cleanup

5c03cf0

Fix: test suite.

3c84dd8

Fix: remove broken test case.

d2bfe97

I think this test had some issue with whitespace being changed after signing, and canonicalization does not touch whitespace that much. I am removing it because it is very likely that this is the issue, and very hard to robustly confirm it.

Add test case: idempotency of canonicalization.

8cde6f3

fisx changed the title ~~[WIP] debug xml dsig~~ Debug xml dsig Feb 13, 2019

neongreen reviewed Feb 13, 2019

View reviewed changes

Cleanup

d267a80

Co-Authored-By: fisx <[email protected]>

fisx merged commit 6789978 into master Feb 13, 2019

fisx deleted the fisx-debug-xml-dsig branch February 13, 2019 20:07

fisx added a commit to wireapp/wire-server that referenced this pull request Feb 13, 2019

bump hsaml2 dep (wireapp/hsaml2#4)

9071eae

fisx mentioned this pull request Feb 13, 2019

bump hsaml2 dep wireapp/wire-server#623

Merged

fisx added a commit to wireapp/wire-server that referenced this pull request Feb 14, 2019

bump hsaml2 dep (wireapp/hsaml2#4) (#623)

c1c9840

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug xml dsig #4

Debug xml dsig #4

fisx commented Feb 12, 2019 •

edited

Loading

ChrisPenner Feb 12, 2019 •

edited

Loading

ChrisPenner Feb 12, 2019 •

edited

Loading

fisx Feb 12, 2019

ChrisPenner Feb 12, 2019

fisx Feb 12, 2019

ChrisPenner Feb 12, 2019

ChrisPenner Feb 12, 2019

fisx Feb 12, 2019

ChrisPenner Feb 12, 2019 •

edited

Loading

ChrisPenner commented Feb 12, 2019

fisx commented Feb 12, 2019

fisx commented Feb 12, 2019

fisx commented Feb 12, 2019

fisx commented Feb 13, 2019

neongreen Feb 13, 2019

neongreen Feb 13, 2019

fisx Feb 13, 2019

neongreen Feb 13, 2019

fisx Feb 13, 2019

neongreen Feb 13, 2019

fisx Feb 13, 2019

neongreen commented Feb 13, 2019

fisx commented Feb 13, 2019

neongreen commented Feb 13, 2019 via email

Debug xml dsig #4

Debug xml dsig #4

Conversation

fisx commented Feb 12, 2019 • edited Loading

ChrisPenner Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

ChrisPenner Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisPenner Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

ChrisPenner commented Feb 12, 2019

fisx commented Feb 12, 2019

fisx commented Feb 12, 2019

fisx commented Feb 12, 2019

fisx commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen commented Feb 13, 2019

fisx commented Feb 13, 2019

neongreen commented Feb 13, 2019 via email

fisx commented Feb 12, 2019 •

edited

Loading

ChrisPenner Feb 12, 2019 •

edited

Loading

ChrisPenner Feb 12, 2019 •

edited

Loading

ChrisPenner Feb 12, 2019 •

edited

Loading