Hi Mike, > On Mon, Mar 10, 2008 at 2:59 PM, Sarah Jelinek <Sarah.Jelinek at sun.com> > wrote: > >> Hi Mike, Jan and Sundar, >> >> Please take a look at my updated webrev for Bug 135 .bashrc owned by root: >> >> http://cr.opensolaris.org/~sjelinek/bashrc_root/ >> > > I like this approach a lot more. > > I'm not worried about this doing the wrong thing in this code path, > but... > > 1959 (void) snprintf(user_path, sizeof (user_path), "%s/%s/%s/%s", > 1960 target, home, save_login_name, bashrc); > 1961 > 1962 (void) snprintf(cmd, sizeof (cmd), > 1963 "/bin/sed -e 's/^PATH/%s &/' %s >%s", > 1964 "export", profile, user_path); > > My inclination (don't know about Sun's real standards around this) > would be to not run sed if snprintf returned a value greater than > sizeof(user_path). That is: > > if (snprintf(user_path, sizeof (user_path), "%s/%s/%s/%s", > target, home, save_login_name, bashrc) > sizeof(user_path)) { > /* log something? */ > return; > } > Actually this is a good point. But, not one I should solve by doing this. In looking at the code for input of login name in the GUI it seems we don't restrict the number of chars that can be input. This could certainly create an overflow situation with user_path. But, the real bug isn't that, it is that we are not restricting the login name length to what the system expects.
So, I took a look at the code that handles the adding of the user to the passwd/shadow files. And we actually have a few things broken with regard to this. We currently: -Fail to add the user if the login name > 9 chars(not including null terminating char) -We log this failure, but continue on with the creation of the users home directory(which could cause the overflow to user_path) -We don't tell the user that the login name they input had a problem during the input of this name for > length. We do check for structure of the login name at input. I assumed, incorrectly, that the login name + /export/home wouldn't exceed > 1024 chars. It wouldn't if we were doing the appropriate checking. I will file a couple of bugs on this to: -Have the GUI enforce the limit on the number of chars allowed for the login name -Do the right thing with regard to the user home directory. If adding the user to the /etc/passwd|shadow files fails then we shouldn't create the home directory. We need to log this issue as well, although I don't think this should be a catastrophic failure for installing. > (void) snprintf(cmd, sizeof (cmd), > "/bin/sed -e 's/^PATH/%s &/' %s >%s", > "export", profile, user_path); > > > And another nit: > > $ cstyle perform_slim_install.c > perform_slim_install.c: 2021: line > 80 characters > > Yeah.. I know about this one. Fixed. thanks for the review, sarah **** > Sorry for missing these on the first pass! > Mike > >
