| Summary: | ASTERISK-05528: DISA passwords in file are not validated - fix provided | ||
| Reporter: | edgreenberg (edgreenberg) | Labels: | |
| Date Opened: | 2005-11-09 12:19:31.000-0600 | Date Closed: | 2008-01-15 15:55:31.000-0600 |
| Priority: | Major | Regression? | No |
| Status: | Closed/Complete | Components: | Applications/app_disa |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ( 0) app_disa.diff ( 1) app_disa.patch | |
| Description: | In bug 5565, wacker reports that DISA fails to authenticate against passwords contained in a file. I fixed this last night, and would like to provide a patch. Discussion below. ****** ADDITIONAL INFORMATION ****** In beta1 and before, the tmp variable, which was used for many purposes was a char buffer of 256. Now, it's a char pointer. This broke the use of it for a buffer in the fgets that iterates over the password file. The failure is that sizeof(tmp) is no longer 256 but rather, 4. I fixed this by adding a new buffer variable for this purpose. I will file the disclaimer and post the patch by close of business today. | ||
| Comments: | By: BJ Weschke (bweschke) 2005-11-09 12:54:30.000-0600 how are you arriving at the conclusion that sizeof(tmp) == 4? By: edgreenberg (edgreenberg) 2005-11-09 13:16:32.000-0600 Disclaimer faxed By: edgreenberg (edgreenberg) 2005-11-09 13:22:54.000-0600 bweschke: Well, when the code runs, fgets returns only two characters of each line on each run, thus, the loop runs way more times than there are lines in the file, and the comparison that would end the loop processing fails. Then we get to the end of the loop and tmp still doesn't contain anything useful to compare to exten. Also, Some of the code in the password section of app_disa is doing things that would make sense to char buffers, but not to char pointers. Finally, tmp is used for so many things, it's just an accident waiting to happen. I think a few more well named pointers would make app_disa much more robust. Maybe sizeof(tmp) isn't 4, but it sure ain't 256. If the powers that be prefer to fix this another way, that's fine with me. I am just trying to shed some light. Best, </edg> By: BJ Weschke (bweschke) 2005-11-09 13:42:08.000-0600 edg: couldn't agree with you more that tmp is being used in too many places. try app_disa.patch and let me know if it works for you. It uses a new method for argument parsing which should cleanup alot of string functions that were in there previously. By: Kevin P. Fleming (kpfleming) 2005-11-10 20:03:10.000-0600 The bugfix patch has been committed to CVS HEAD. The parsing conversion will have to wait until after 1.2. Thanks! By: Digium Subversion (svnbot) 2008-01-15 15:55:31.000-0600 Repository: asterisk Revision: 7065 U trunk/ChangeLog U trunk/apps/app_disa.c ------------------------------------------------------------------------ r7065 | kpfleming | 2008-01-15 15:55:30 -0600 (Tue, 15 Jan 2008) | 2 lines issue ASTERISK-5528 ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=7065 | ||