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

afpd: Prevent a potential buffer overflow when changing password #628

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

neko-hat
Copy link
Contributor

@neko-hat neko-hat commented Dec 26, 2023

It seems unlikely that a bug will occur, but it seems that a patch is needed just in case.

diff --git a/etc/afpd/auth.c b/etc/afpd/auth.c
index 15e16441..89a2a3b3 100644
--- a/etc/afpd/auth.c
+++ b/etc/afpd/auth.c
@@ -827,7 +827,7 @@ int afp_logout(AFPObj *obj, char *ibuf _U_, size_t ibuflen  _U_, char *rbuf  _U_
  */
 int afp_changepw(AFPObj *obj, char *ibuf, size_t ibuflen, char *rbuf, size_t *rbuflen)
 {
-    char username[MACFILELEN + 1], *start = ibuf;
+    char username[MAXUSERLEN], *start = ibuf;
     struct uam_obj *uam;
     struct passwd *pwd;
     size_t len;

It could be overflowed when uam_getname is called in afp_changepw. (that function is in etc/afpd/uam.c)
(name buf size is 32 but MAXUSERLEN is 255)

etc/afpd/auth.c Outdated Show resolved Hide resolved
@neko-hat neko-hat requested a review from rdmark December 27, 2023 02:48
@neko-hat
Copy link
Contributor Author

@rdmark I completed the revision by reflecting your feedback.
Please review it.

@rdmark
Copy link
Member

rdmark commented Dec 27, 2023

@neko-hat I think the code looks good now, thanks.

Now, I have one more ask for you: In this project we have policies for the changeset and commit messages. The goal is to have a descriptive and high quality historical commit log in the project. So, can you please squash this changeset into one commit, and write a descriptive commit message in the format described in the below wiki page?

https://github.com/Netatalk/netatalk/wiki/Developer-Notes

Basically, the commit message should say something like "afpd: Use correct username length in afp_changepw, Issue #XYZ" (with the actual issue number)

Are you familiar with how to reset the HEAD and force push in git?

@neko-hat
Copy link
Contributor Author

@rdmark OK, I'm going to try it now

@neko-hat
Copy link
Contributor Author

@rdmark I'm sorry I'm not good at it. I changed it as you said, is it okay?

Copy link
Member

@rdmark rdmark left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@rdmark rdmark merged commit 3ef2274 into Netatalk:main Dec 27, 2023
3 checks passed
@rdmark rdmark changed the title Fixing bugs that can cause system errors afpd: Prevent a potential buffer overflow Jun 2, 2024
@rdmark rdmark changed the title afpd: Prevent a potential buffer overflow afpd: Prevent a potential buffer overflow when changing password Jun 2, 2024
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