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

[16.0][FIX] users_ldap_groups: safe LDAP decode #596

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Conversation

oh2fih
Copy link
Contributor

@oh2fih oh2fih commented Dec 25, 2023

The group mapping in query mode fails if LDAP returns binary data in any of the fields. This adds a function that handles such situation by base64 encoding it.

@oh2fih oh2fih changed the title [FIX] mig-users_ldap_groups: safe LDAP decode [FIX] users_ldap_groups: safe LDAP decode Dec 25, 2023
@oh2fih oh2fih changed the title [FIX] users_ldap_groups: safe LDAP decode [16.0][FIX] users_ldap_groups: safe LDAP decode Dec 25, 2023
@oh2fih
Copy link
Contributor Author

oh2fih commented Dec 25, 2023

This same bug affects all other versions, too.

I'm about to give up writing the test coverage for this change because codecov keeps complaining about everything I try. (Especially I didn't understand why it demands test coverage for the tests, too.) My idea is to add binary data to the LDAP response, e.g., a minimal GIF image as the LDAP attribute thumbnailPhoto, as this is a common case where LDAP returns binary data.

{
    "dc=users_ldap_groups,dc=example,dc=com": {
        "cn": [b"User Name"],
        "name": [b"hello", b"hello2"],
        "thumbnailPhoto": [
            b"GIF89a\x01\x00\x01\x00\x00\xff\x00,"
            b"\x00\x00\x00\x00\x01\x00\x01\x00\x00\x02\x00;"
        ],
    }
}

@oh2fih
Copy link
Contributor Author

oh2fih commented Jan 2, 2024

Tested with Odoo 16.0.20240102 on Ubuntu 22.04.

@oh2fih oh2fih changed the title [16.0][FIX] users_ldap_groups: safe LDAP decode [16.0][FIX] users_ldap_groups: safe LDAP decode & fix JSON RPC vulnerability Feb 23, 2024
@oh2fih oh2fih force-pushed the 16.0 branch 14 times, most recently from 789dafd to 080ae25 Compare March 30, 2024 07:52
@oh2fih oh2fih closed this Mar 30, 2024
Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no ldap specialist at all, but I understand the vulnerability and your fix seems both reasonable and safe so I approve it. Let's give it a couple more days to see if we get another reviewer. If not I will be able to merge it.

But yeah the ldap module isn't very actively maintained and possibly the maintainers just missed your issue. You could eventually propose yourself as a module maintainer (adding your GitHub user to the maintainers value of the module manifest file in a PR will be enough to enable to let you merge and publish improvements in the module).

Thank you for the fix.

@pedrobaeza pedrobaeza added this to the 16.0 milestone Jun 1, 2024
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this fix.

For next time, note security@odoo-community.org may be contacted for reporting such issues.

@sbidoul
Copy link
Member

sbidoul commented Jun 4, 2024

This decode-that-encodes-in-base64 looks a bit weird to me. Why is the decode necessary in the first place?

Perhaps, to move faster, you could do the security fix in a PR, so I can merge this, and do the decode improvement in another PR and work on that one with folks who are more familiar with this module?

@oh2fih
Copy link
Contributor Author

oh2fih commented Jun 4, 2024

This decode-that-encodes-in-base64 looks a bit weird to me. Why is the decode necessary in the first place?

This simply replaces the original ldap_entry[1][attr][0].decode() doing the fix with the least possible modifications to the module. The Str.decode() was added in ad15e3a when migrating the module from Odoo 10 to Odoo 12 and seems quite necessary, because it did not work without it on the first attempts to address this bug. The contains & equals uses the Str.decode() (in x.decode()), too, so not using it here does not seem coherent, either. Although not explained in the commit, I guess the root cause might be support for non-ASCII names.

So, why not use this safe_ldap_decode() in contains & equals then? Because only the requested LDAP attributes are compared and they should never contain binary data. This is a special case for the query operator for which LDAP returns the entire Active Directory object with all the attributes.

Does this satisfactorily explain the situation here?

@oh2fih oh2fih changed the title [16.0][FIX] users_ldap_groups: safe LDAP decode & fix JSON RPC vulnerability [16.0][FIX] users_ldap_groups: safe LDAP decode Jun 6, 2024
@oh2fih
Copy link
Contributor Author

oh2fih commented Jun 6, 2024

Perhaps, to move faster, you could do the security fix in a PR

@sbidoul: There's now another PR #659 that only contains the commit fixing the vulnerability. After merging that this PR is only the second commit ahead of upstream 16.0 branch.

@oh2fih
Copy link
Contributor Author

oh2fih commented Jul 19, 2024

Rebased to fix the checks that didn't pass; the problem was in another module.

Could someone please review & merge this already?

The group mapping in query mode fails if LDAP returns binary data in any
of the fields. This adds a function that handles such situation by
base64 encoding it.
The new test test_users_ldap_groups_ldap_returns_binary_data covers
the common case where LDAP return binary data in thumbnailPhoto.
@oh2fih
Copy link
Contributor Author

oh2fih commented Sep 22, 2024

Rebased over fc906889df964a3f8b50013389dbc585aee0080a after workflow fixed in #693.

@oh2fih
Copy link
Contributor Author

oh2fih commented Sep 22, 2024

@sbidoul It seems difficult to get anyone that actually knows something about LDAP to review this fix. However, this has been running on an instance for months and has fixed a real-life problem there. The problematic LDAP response has been demonstrated in a new test this PR adds, and all the previous tests are passing, too. Could this finally be merged?

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-596-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit fa19c09 into OCA:16.0 Sep 23, 2024
9 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at d4fee8b. Thanks a lot for contributing to OCA. ❤️

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

Successfully merging this pull request may close these issues.

6 participants