Change namespace separator #78

SlavekB · 2022-02-24T19:44:59Z

Three parts are addressed:

Crash in case of damaged wbxml file.
Hard-coded colon as a namespace separator in SyncML related code.
Change the default namespace separator.

See previous comments on issue #76.

hartwork · 2022-02-24T19:58:51Z

src/wbxml_parser.c

@@ -2479,7 +2479,8 @@ static WBXMLError decode_opaque_content(WBXMLParser  *parser,
    case WBXML_LANG_SYNCML_SYNCML11: 
    case WBXML_LANG_SYNCML_SYNCML12: 
        /* NextNonce */
-        if ((parser->current_tag->wbxmlCodePage == 0x01) &&
+        if ((parser->current_tag) &&


@SlavekB thanks for the PR, very nice! 👍

I tried running the tests without that^^ part of the patch and everything still passes. So it seems that part is not strictly related. I would like to vote to make that a separated pull request and to move it out of here for clarity, but just my two cents. Thanks a lot for working on this! 👍

This crash occurred when a damaged wbxml files was generated during tests. Therefore I put it as part of this PR. Because the second commit in this PR fixes problem with damaged wbxml files, the crash does not occur now.

I would like to keep as it makes the code more robust.

bellmich · 2022-02-24T20:14:06Z

@SlavekB - can you please add a note to ChangeLog and your credits to the file THANKS too, please?

bellmich · 2022-02-24T20:22:33Z

A more general question, I am wondering what this change means in terms of SO naming for the generated library. Any opinions or hints, please?

hartwork · 2022-02-24T21:01:37Z

A more general question, I am wondering what this change means in terms of SO naming for the generated library. Any opinions or hints, please?

@bellmich depends on the other changes since the last release. This PR in isolation would probably just a bump of revision (from current 7 to 8), since it does not break API or ABI. https://verbump.de/ may also be of help.

Related code: https://github.com/libwbxml/libwbxml/blob/master/CMakeLists.txt#L16-L33

SlavekB · 2022-02-25T00:07:56Z

Because the change of the namespace separator's is only internal and does not cause a change in API / ABI, it seems to me that there is no need to change LIBWBXML_LIBVERSION_REVISION, but that it is sufficient to increase LIBWBXML_VERSION_PATCH. This seems that it is not an established rule that both values must be changed simultaneously.

If there is consent, I should prepare a record to ChangeLog for release 0.11.8 and increase the value of LIBWBXML_VERSION_PATCH in CMakeLists.txt? Or information about release 0.11.8 I should keep on you?

Signed-off-by: Slávek Banko <slavek.banko@axis.cz>

instead of hardcoded colon in the SyncML related code. Signed-off-by: Slávek Banko <slavek.banko@axis.cz>

SlavekB · 2022-02-25T00:22:10Z

Rebased to current head and added note to ChangeLog.

hartwork · 2022-02-25T00:22:22Z

Because the change of the namespace separator's is only internal and does not cause a change in API / ABI, it seems to me that there is no need to change LIBWBXML_LIBVERSION_REVISION, but that it is sufficient to increase LIBWBXML_VERSION_PATCH. This seems that it is not an established rule that both values must be changed simultaneously.

@SlavekB I think we have a misunderstanding here. I agree that bumping LIBWBXML_VERSION_PATCH (the 7 in 0.11.7) to 8 may make a good project version bump, but the soversion of the library (which @bellmich asked about) is moving independently of the project version, and will need some form of bump with the next release because (quoting (complex) related libtool docs):

"If the library source code has changed at all since the last update, then increment [..]."

How to bump the soversion is non-trivial — there is an algorithm —, and is a key reason I created (1) https://verbump.de/ for help and (2) shared a simpler algorithm for anyone who likes to do it with "pen and paper" at https://blog.hartwork.org/posts/shared-library-version-bump/ . I'm hoping that at least one of these two can help answer the question what to bump current 1:7:0 to.

ChangeLog

SlavekB · 2022-02-25T00:30:53Z

Yes, the version numbering of SO libraries is known to me. Therefore, I believe that this change is not necessary to be reflected on the soversion of the library, because it does not make any change of the library interface.

… pipe. This solves compatibility with libexpat >= 2.4.5 after fix the security problem CVE-2022-25236. This resolves issue libwbxml#76. Signed-off-by: Slávek Banko <slavek.banko@axis.cz>

hartwork · 2022-02-25T00:39:13Z

Yes, the version numbering of SO libraries is known to me. Therefore, I believe that this change is not necessary to be reflected on the soversion of the library, because it does not make any change of the library interface.

@SlavekB if it is known to you, then you will agree that changes in source need a bump in soversion, once before the next release. What am I missing?

hartwork · 2022-02-25T00:43:54Z

@SlavekB with the help of https://verbump.de/ :

SlavekB · 2022-02-25T01:22:35Z

Yes, the version numbering of SO libraries is known to me. Therefore, I believe that this change is not necessary to be reflected on the soversion of the library, because it does not make any change of the library interface.

@SlavekB if it is known to you, then you will agree that changes in source need a bump in soversion, once before the next release. What am I missing?

You can notice in the git history that changes are not always reflected on LIBWBXML_LIBVERSION_REVISION.

From one view, this is a change that affects only internal behavior. From the second view, it provides compatibility with a newer version of libexpat, so it seems that it could be a good idea to be reflected on LIBWBXML_LIBVERSION_REVISION. I will keep the decision on @bellmich

hartwork · 2022-02-25T01:30:34Z

@SlavekB we seem to be on different boats with soversion'ing. I have said what I wanted to say, and don't want a fight about it or start repeating myself, so I will not reply to that last message in detail and just hope that whoever does the release does the right thing.

SlavekB · 2022-02-25T12:37:36Z

I was thinking again. I agree that it seems more good reasons to increase LIBWBXML_LIBVERSION_REVISION as it suggests @hartwork. I assume that this makes @bellmich along with the increase in LIBWBXML_VERSION_PATCH and official release.

Thank you for your revisions and comments.
Is there anything else I should do?

hartwork · 2022-02-25T15:22:34Z

@SlavekB thanks for your last comment. I'm not aware of anything else needed. Thanks for your work on this pull request! 👍

bellmich · 2022-02-25T19:46:07Z

Thanks for all the comments regarding the SO naming and sorry for being late to the party ;-) Potentially, I should add more context to my question right from the beginning. I know that the changes are fully API and ABI compatible with the version before and now comes the big but: the behaviour of already existing functions changed. If I play this really strict, it must be interpreted like removing and adding a new function call. The problem is, if I do this and change the SO versions accordingly, all old binaries would not use the new library and essentially as this is a security fix, they would stay vulnerable.

Therefore, I think about to just increase REVISION and PATCH like you proposed to make the fix available to old binaries. My problem is, I have no clue what the maintainers from the distributions think about this and if I break any program with the changed namespace separator.

bellmich · 2022-02-25T20:38:58Z

As the pull request works perfectly, I'll merge it. Thanks a lot for your help!

SlavekB · 2022-02-26T22:26:59Z

Please, do you already have a plan to release 0.11.8 containing these patches?

bellmich · 2022-02-27T08:01:13Z

Please, do you already have a plan to release 0.11.8 containing these patches?

Yes, I contacted some people from Debian and asked for their opinion on the SO naming. I have no final answer so far but they are aware of the Expat security issue, the Expat SO naming and the resulting trouble.

bellmich · 2022-02-27T08:02:35Z

@SlavekB - Do you have a deadline or pressing need for some reason (e.g. publicly available vulnerable system or something like this)?

bellmich · 2022-02-27T08:03:32Z

Potentially beside the SO naming, the required Expat version could/should be enforced.

SlavekB · 2022-02-27T09:21:32Z

@SlavekB - Do you have a deadline or pressing need for some reason (e.g. publicly available vulnerable system or something like this)?

My interest in the new version is because of the fact that the Expat security update caused non-functional ActiveSync synchronization in SOGo – see: Bug 1006337 in Debian.

In addition to official packages in Debian, I provide community repository with packages for the latest version of SOGo, so I'm interested in building libwbxml packages for this repository – see axis repository for SOGo.

hartwork · 2022-02-27T14:47:43Z

I know that the changes are fully API and ABI compatible with the version before and now comes the big but: the behaviour of already existing functions changed. If I play this really strict, it must be interpreted like removing and adding a new function call.

@bellmich could you elaborate how the internal change will affect users of libwbxml? If they never got to see strings containing colon as a separator returned by some API call, then the change is purely internal and not a breaking change. Is there any place like that in libwbxml?

Potentially [..] the required Expat version could/should be enforced.

@bellmich that seems unnecessary, it may even hurt people with backported patches where the version number is lower. Could you elaborate on the idea?

hartwork · 2022-02-27T14:50:22Z

@SlavekB FYI your patch is used in Gentoo by now (gentoo/gentoo@8b88021)

bellmich · 2022-02-27T16:31:18Z

I know that the changes are fully API and ABI compatible with the version before and now comes the big but: the behaviour of already existing functions changed. If I play this really strict, it must be interpreted like removing and adding a new function call.

@bellmich could you elaborate how the internal change will affect users of libwbxml? If they never got to see strings containing colon as a separator returned by some API call, then the change is purely internal and not a breaking change. Is there any place like that in libwbxml?

If the library is used to convert WBXML into XML, the default namespace separator changed and this way, the XML output changed. The question is, are there any XML parsers who could be in trouble with the new namespace separator? As I didn't receive any feedback from Debian so far, I tend to go with your proposal, @SlavekB.

Potentially [..] the required Expat version could/should be enforced.

@bellmich that seems unnecessary, it may even hurt people with backported patches where the version number is lower. Could you elaborate on the idea?

The idea came to me because it potentially help to reach a consistent security state. The new version would enforce an update to a secure version of Expat. Otherwise, a libwbxml version with a security fix could still lead to a vulnerable system because it can be used with a vulnerable Expat. If the idea is a bad one, I am open for feedback. I am just very over-carefully because it is a security fix. Sorry, if it sounds a little bit silly.

hartwork · 2022-02-27T17:03:28Z

If the library is used to convert WBXML into XML, the default namespace separator changed and this way, the XML output changed.

@bellmich let's be sure we don't mixup two things under the term "namespace separator". We have:

the namespace separator colon (':') in XML payload — e.g. twice in <prefix1:tag1 xmlns:prefix2="[..]"> — that can be nothing but a colon, as defined by the XML 1.0 namespaces spec, and we have
the namespace separator passed to Expat function XML_ParserCreateNS and XML_ParserCreate_MM that has some flexibility and is tell Expat what character to put between the namespace URI and the local tag name when passing the result to e.g. element start handlers.

So where libwbxml is writing XML, it can be nothing but colon, and in element handlers and in internal communication it has some flexibility to use e.g. a pipe ('|') character. My understand of this pull request here is that it changes the reading side of things but not the writing side. Does that make sense?

CC @SlavekB

SlavekB · 2022-02-27T17:19:10Z

My observation is that changing the separator has only an internal impact => this does not change the outputs to wbxml, nor to readable xml. And it does not seem to be some public API functions that would be affected by changing the separator.

Therefore, my observation is that the change has no impact on any library public interface.

SlavekB · 2022-02-27T17:44:11Z

Potentially [..] the required Expat version could/should be enforced.

@bellmich that seems unnecessary, it may even hurt people with backported patches where the version number is lower. Could you elaborate on the idea?

The idea came to me because it potentially help to reach a consistent security state. The new version would enforce an update to a secure version of Expat. Otherwise, a libwbxml version with a security fix could still lead to a vulnerable system because it can be used with a vulnerable Expat. If the idea is a bad one, I am open for feedback. I am just very over-carefully because it is a security fix. Sorry, if it sounds a little bit silly.

Require a specific version of Expat containing security patches at build time does not seem good idea:

This does not ensure that the binary package will require such versions because the dependence between libraries normally requires a version according to API / ABI of library – regardless of patches that do not have a consequence to change the API / ABI.
This causes complications for distribution, where security patches are commonly backported to previous versions of libraries, so the basic version of the library remains unchanged.

The settings of dependency for the binary packages is beyond the source code, so irrelevant to release a new version.

bellmich · 2022-02-27T17:52:08Z

@SlavekB & @hartwork, I will release in the evening or tomorrow a new version with your proposed SO versioning. I just want to fix issue #80 too to avoid more confusion. I hope that's okay for you. Thanks a lot for your help, patience and clarification support.

bellmich · 2022-02-28T20:54:55Z

@SlavekB & @hartwork, FYI I released libwbxml 0.11.8. Thanks a lot for your help!

SlavekB · 2022-02-28T21:43:52Z

@SlavekB & @hartwork, FYI I released libwbxml 0.11.8. Thanks a lot for your help!

Thank you, I am already working on updating the package in my community repository for SOGo.

hartwork reviewed Feb 24, 2022

View reviewed changes

hartwork mentioned this pull request Feb 24, 2022

Vulnerable Expat 1.95.6 bundled at macosx/expat/ #77

Closed

hartwork mentioned this pull request Feb 24, 2022

[>=2.4.5] Fix to CVE-2022-25236 breaks biboumi, ClairMeta, jxmlease, libwbxml, openleadr-python, rnv, xmltodict libexpat/libexpat#572

Closed

SlavekB added 2 commits February 25, 2022 01:15

Fix crash on corrupted wbxml.

186791d

Signed-off-by: Slávek Banko <slavek.banko@axis.cz>

Allow the use of a namespace separator specified by the constant

4664d47

instead of hardcoded colon in the SyncML related code. Signed-off-by: Slávek Banko <slavek.banko@axis.cz>

SlavekB force-pushed the issue/76/change-namespace-separator branch from f171cb0 to acc8912 Compare February 25, 2022 00:21

hartwork reviewed Feb 25, 2022

View reviewed changes

ChangeLog Outdated Show resolved Hide resolved

Change the default internal namespace separator from the colon to the…

4425e80

… pipe. This solves compatibility with libexpat >= 2.4.5 after fix the security problem CVE-2022-25236. This resolves issue libwbxml#76. Signed-off-by: Slávek Banko <slavek.banko@axis.cz>

SlavekB force-pushed the issue/76/change-namespace-separator branch from acc8912 to 4425e80 Compare February 25, 2022 00:32

hartwork approved these changes Feb 25, 2022

View reviewed changes

bellmich merged commit 9e3489a into libwbxml:master Feb 25, 2022

SlavekB deleted the issue/76/change-namespace-separator branch February 25, 2022 21:18

hartwork mentioned this pull request Feb 27, 2022

WBXML_NAMESPACE_SEPARATOR ':' incompatible with Expat >=2.4.5 security fixes #76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change namespace separator #78

Change namespace separator #78

SlavekB commented Feb 24, 2022

hartwork Feb 24, 2022

SlavekB Feb 24, 2022

bellmich Feb 24, 2022

bellmich commented Feb 24, 2022

bellmich commented Feb 24, 2022

hartwork commented Feb 24, 2022

SlavekB commented Feb 25, 2022

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022

hartwork commented Feb 25, 2022

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022 •

edited

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022

bellmich commented Feb 25, 2022

bellmich commented Feb 25, 2022

SlavekB commented Feb 26, 2022

bellmich commented Feb 27, 2022

bellmich commented Feb 27, 2022

bellmich commented Feb 27, 2022

SlavekB commented Feb 27, 2022

hartwork commented Feb 27, 2022

hartwork commented Feb 27, 2022

bellmich commented Feb 27, 2022

hartwork commented Feb 27, 2022

SlavekB commented Feb 27, 2022

SlavekB commented Feb 27, 2022

bellmich commented Feb 27, 2022

bellmich commented Feb 28, 2022

SlavekB commented Feb 28, 2022

Change namespace separator #78

Change namespace separator #78

Conversation

SlavekB commented Feb 24, 2022

hartwork Feb 24, 2022

Choose a reason for hiding this comment

SlavekB Feb 24, 2022

Choose a reason for hiding this comment

bellmich Feb 24, 2022

Choose a reason for hiding this comment

bellmich commented Feb 24, 2022

bellmich commented Feb 24, 2022

hartwork commented Feb 24, 2022

SlavekB commented Feb 25, 2022

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022

hartwork commented Feb 25, 2022

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022 • edited

SlavekB commented Feb 25, 2022

hartwork commented Feb 25, 2022

bellmich commented Feb 25, 2022

bellmich commented Feb 25, 2022

SlavekB commented Feb 26, 2022

bellmich commented Feb 27, 2022

bellmich commented Feb 27, 2022

bellmich commented Feb 27, 2022

SlavekB commented Feb 27, 2022

hartwork commented Feb 27, 2022

hartwork commented Feb 27, 2022

bellmich commented Feb 27, 2022

hartwork commented Feb 27, 2022

SlavekB commented Feb 27, 2022

SlavekB commented Feb 27, 2022

bellmich commented Feb 27, 2022

bellmich commented Feb 28, 2022

SlavekB commented Feb 28, 2022

hartwork commented Feb 25, 2022 •

edited