Summary:	ASTERISK-06174: [patch] Asterisk ignores ACLs and umask for most file creations
Reporter:	Ben Klang (bklang)	Labels:
Date Opened:	2006-01-23 15:14:48.000-0600	Date Closed:	2006-12-21 14:22:41.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) asterisk-file-mode-retry-v2.diff ( 1) ast-file-mode-config.patch ( 2) ast-file-mode-r29695.patch ( 3) ast-file-mode-r41810.patch
Description:	I came across this issue while trying to understand why my default ACLs were not being applied to new voicemails as they were spooled into the directory.  This problem appears to have been partially solved with the addition of VOICEMAIL_FILE_MODE and VOICEMAIL_DIR_MODE in apps/app_voicemail.c but I wanted to see the fix made more consistent across the tree. I looked for all instances where open() would be called with O_CREATE and changed the mode arg to AST_FILE_MODE which I then defined in include/asterisk.h This tweak should allow the a system administrator to use umask and ACL entries as they were intended rather than having the application stomp over those configurations.  If a particular distribution wishes to tighten default perms they can do so easily by modifying include/asterisk.h. Note I did not modify the behavior of apps/app_voicemail.c's VOICEMAIL_FILE_MODE as it appeared sufficient and non-conflicting to me.
Comments:	By: Ben Klang (bklang) 2006-01-23 15:16:11.000-0600 My internet connection appears to have burped while posting.  That patch may not be complete.  I will upload a new one. By: Ben Klang (bklang) 2006-01-23 15:20:13.000-0600 The file appears intact but for paranoia sake here's another copy By: Kevin P. Fleming (kpfleming) 2006-02-14 19:43:00.000-0600 Conceptually, I agree with this completely... we should not be forcing permissions on any files we create, except for temp files. The system administrator can control permissions of the resulting files using umask, ACLs, whatever. However, this will be a significant change to make, and the ramifications could go on for quite some time. I think we need to get some consensus from the community (via the asterisk-dev mailing list) before proceeding. Also, remove the comment with the date and your email address. The coding guidelines specifically state that comments that just give someone credit for things are not acceptable. By: Ben Klang (bklang) 2006-02-16 10:20:07.000-0600 Apologies, the comment has been removed. A new patch is attached which removes the comment and updates to the latest trunk (2006/02/16 at 12:15EST). I will be posting to -dev regarding this patch as you suggest. Thanks! By: Ben Klang (bklang) 2006-02-22 11:37:51.000-0600 After collecting replies from the -DEV list for 6 days, here is the result: 4 responses with 2 apparently in favor.  The other 2 raised questions which I believe I was able to provide sufficient answers for.  In any case the answers went unchallenged.  This patch has now been moved to production in my environment and no side-effects have been noted. What steps are necessary to get this patch considered for inclusion into trunk? By: Thomas Meier (thmeier) 2006-02-25 16:10:39.000-0600 I habe used this patch because I need to read voice-mail files from an Apache-PHP application. The patch is working as aspected and the vm-files have the rights 644. I will deeper test it for some days and will use it on an productive server. Thanks for this patch! By: Thomas Meier (thmeier) 2006-02-26 02:11:05.000-0600 If an voicemailbox dir is not exist Asterisk will create an new dir for this user in /var/spool/asterisk/voicemail/$section. This new generated dir has rights 700 and the files in this dir have 644. The dir should have 755 so that user others can access the files in this dir. By: Olle Johansson (oej) 2006-04-05 10:34:16 I think this should be a config option in asterisk.conf. We already have "astctlpermissions" as a setting, might as well add "astfilepermissions" for creating files. By: Olle Johansson (oej) 2006-04-05 10:35:07 If you have time to add a config option, I'll include this in the test branch for testing. By: Serge Vecher (serge-v) 2006-05-04 10:15:06 bklang: can you please update your patch implementing an option as requested by Olle, please? Thanks By: Ben Klang (bklang) 2006-05-04 10:17:14 I will try to get an updated patch back by this weekend based on the recommendations in this ticket. /BAK/ By: Ben Klang (bklang) 2006-05-10 00:19:57 I have updated the patch to tonight's SVN trunk.  I have also implemented the requested functionality to allow specifying the file creation mode in asterisk.conf.   This patch has been tested as working on a Linux system, although not nearly exhaustively.  I can verify that the expected behavior was observed from the voicemail system both when specifying a file mode in asterisk.conf and also when not specifying anything in asterisk.conf. A couple things came up:   *  I had to add three header files to the __linux__ block of includes/asterisk/compat.h to get this to build.  I'm not entirely clear this was the right thing to do but it was the most obvious thing I saw.  This likely means this patch may cause a build problem on non-Linux platforms.  I attempted to make a similar correction on the Darwin platform but I am unable to test building on that at the moment. * There are still some files still being created that are not controlled by this patch.  Notably the .txt control file for voicemails in apps/app_voicemail.c but I haven't yet tracked down the cause.  I also made no attempts to modify the db1 libs included as they do not seem to inherit the ast_config variables. * I changed the type of ast_config_AST_CTL_PERMISSIONS to match ast_config_AST_FILE_PERMISSIONS; both are now of type mode_t.  This seemed more intuitive to me and ast_config_AST_CTL_PERMISSIONS was only used in one place in the code anyway.  I hope this change is permissible. I welcome any and all feedback!  Please let me know if this will be included in the oej/test-this-branch.  I think this patch will give sysadmins much greater flexibility over the security of their files.  I personally will be using with ACLs to get the functionality I desire. Thanks! /BAK/ By: Ben Klang (bklang) 2006-05-23 01:51:33 17:12:47 <@kpfleming> ok. i talked to mark 17:13:09 <@kpfleming> he's not keen on changing the permissions stuff without reviewing each place that is setting permissions so he can try to explain why it is doing it 17:13:36 <@kpfleming> i don't have time to make the list of all those places, and i'm not sure it makes sense to mess with it at this late date if he's not going to make a blanket statement like 'pull them out' 17:13:57 <@kpfleming> however.. if you want to make that 'review' list for me, at least we'd have a chance to change it, and be able to document the resulting differences in behavior17:14:24 < bklang> I see...ok so you need a list of all places where files are created with perms less than what I am proposing? 17:14:43 < bklang> some of the perms weren't actually changed (a minority, unfortuantely) but were replaced with the config option 17:15:02 < bklang> and others I removed the umask flag on opens that didn't call O_CREAT as required by the manpage 17:15:10 < bklang> I assume those modifications are unchallenged? 17:16:40 < bklang> kpfleming: is there any chance that at least some of this patch will make it into 1.4 or is this entire issue looking at Post-1.4? 17:17:02 <@kpfleming> without an idea of how bad it is, i'd hate to make a guess 17:17:40 < bklang> I guess what I'm getting at is, can I make mods in some of themore annoying places with the hope of those getting into 1.4 with more changes in the future 17:20:12 < bklang> it's time for me to hit the road but I'll work on some information for you tonight.  I'd really like to make some progress on this before 1.4 17:20:29 <@kpfleming> i don't want to add config options, no 17:20:44 <@kpfleming> if we do that, then all the places that don't have them will be 'bugs' against 1.4 17:20:50 < bklang> so you prefer I would revert back to the #define in asterisk.h? 17:20:54 <@kpfleming> we can talk tomorrow then, thanks for your time 17:21:05 <@kpfleming> i don't honestly know 17:21:10 < bklang> hm ok 17:21:11 <@kpfleming> my personal preference is to rip it all out 17:21:22 < bklang> what do you mean by "rip it all out" 17:21:42 <@kpfleming> no permissions setting in asterisk at all 17:21:56 < bklang> well when you call open with O_CREAT you have to say something 17:22:05 < bklang> which is why I chose 666, that is effectively "ripping it out" 17:22:16 <@kpfleming> and that should be 0777, and let umask deal with, right? 17:22:17 < bklang> it moves the decision back to the umask 17:22:26 <@kpfleming> ok, yeah, 0666 17:22:28 <@kpfleming> whichever 17:22:29 < bklang> well 777 on files leaves them executable 17:23:05 < bklang> kpfleming: so the decision is whether to standardize on putting 666 in open() calls with O_CREAT or putting it into a #define? 17:23:35 <@kpfleming> bklang: sounds like it, yes 17:23:52 < bklang> kpfleming: ok for now I'll go back to the #define as that's much easier to search and replace later :) 17:24:01 <@kpfleming> heh 17:24:18 < bklang> ok, I'm off, thanks for the feedback By: Ben Klang (bklang) 2006-05-23 15:44:25 Per the conversation with kpfleming, I have begun to justify the changes made on a file-by-file basis.  These justifications apply to the newest attached patch: ast-file-mode-config-r29695.patch.  This patch affects the files justified below and has been updated to trunk rev 29695. Please post any feedback you have. * channels/chan_zap.c :9188 - Remove unnecessary mode bits (O_CREAT not flagged) :9305 - Remove unnecessary mode bits (O_CREAT not flagged) * chanels/chan_iax2.c :1327 - Creates temporary file for firmware being sent to iaxy.  The file only exists in the filesystem for a very short amount of time and permissions are probably not at all important on this file. * app.c :554 - In function __ast_play_and_record().  Pre-patch the mode is set to 0700, so no ACLs or umask settings will take effect yet still marks the audio file executable.  After my patch, Asterisk uses 666, effectively leaving the mode of the newly created audio file up to the umask or ACLs. :926 - in function ast_lock_path(); affects the permissions of created lock files.  Before the patch the file is created 0700, again creating an exectuable file.  After my patch, Asterisk does not enforce any file permissions (see previous). apps/app_chanspy.c :444 - In function chanspy_exec(), file is created to hold spied-upon audio.  Before my patch, Asterisk enforces 644 (enforced no write for group/other) which in my mind makes no sense.  Most default umasks will acheive this by default so no reason to enforce.  After my patch, Asterisk does not enforce permissions (see above). apps/app_dictate.c :154 - In function dictate_exec(), file is created to hold dictated audio. :311 - In function dictate_exec(), file is cleared and created to hold dictated audio. In both cases above, before the patch Asterisk creates the files 0700.  This again sets the executable bit on audio files while stripping the group and other flags.  After my patch Asterisk will not enforce any file permissions on the audio. apps/app_festival.c :420 - In function festival_exec(), file is created to cache festival audio output.  Before the patch, the audio is created 777, marking the audio as executable.  After my patch the mode is 0666, which acheives the same permissions without marking the audio file executable. apps/app_mixmonitor.c :184 - In function mixmonitor_thread(), file is created to hold mixed audio output.  Before my patch the file is created 0644, after my patch Asterisk does not enforce stripping of group and other write bits.  Again 0644 makes no sense as the default umask covers this. apps/app_meetme.c :2344 - In function recordthread(), file is created to hold recorded audio stream from conference.  Before the patch the file is created 644; after patch Asterisk does not strip group and other write bits.  Again 0644 makes no sense (see above). apps/app_voicemail.c :923 - This patch only makes app_voicemail.c consistent with itself; it already defines a VOICEMAIL_FILE_MODE above which is used in the other case where O_CREAT is used in an open() call.  VOICEMAIL_FILE_MODE is set to 0660 This change is very likely going to be the most visible to admins and end-users.  Since 0660 leaves the group read and write bits intact, the ACLs "default" setting is allowed to take effect while still stripping "other" from having any read access.  I feel this strikes a fair balance between paranoia and still allowing the administrator to use ACLs to determine file permissions.  However it does set the precedent that most other software does not similarly set: on "normal" files created the daemon is determining (to at least some degree) the mode of new files being created. In my opinion this is better handled by modifying scripts/asterisk/safe_asterisk to set a more restrictive umask by default (perhaps 0027) and adding a note to doc/security.txt instead of having Asterisk always remove all "other" permission bits. apps/app_record.c :242 - in function record_exec(), thie file is created to hold recorded audio.  Again pre-patch Asterisk uses 644. See above for justification. db.c :67 - In function dbinit(), the file is created to hold the Asterisk database.  Again, pre-patch Asterisk uses 644.  See above for justification. include/asterisk.h is modified to contain AST_FILE_MODE which is the placeholder until we determine whether the new standard will be to use 0666 in open() calls with O_CREAT or not. res/res_monitor.c :199 - In function ast_monitor_start(), two files are created to hold each leg of the audio.  Again, pre-patch Asterisk uses 0644.  See above for justification. res/res_agi.c :896 - In function handle_recordfile(), the file is created to hold recorded audio.  Again, pre-patch Asterisk uses 0644.  See above for justification. res/res_convert.c :91 - In function cli_audio_convert(), the file is created to hold converted audio output.  Again, pre-patch Asterisk uses 0644.  See above for justification. file.c :220 - In function copy().  This change is perhaps the most significant.  I have not yet audited every location where copy() is used.  However pre-patch Asterisk uses a *very* restrictive mode of 0700.  All files will be created with the execute bit regardless of type.  Also any system-administrator provided umask bits or ACL settings will be ignored (default mask values are calculated against the group permissions when a file is created, at least in Linux).  My preference is to modify contrib/scripts/safe_asterisk to include a tighter umask and to modfiy doc/security.txt rather than hamstring the system administrator with such a restrictive file mask. cdr/cdr_sqlite.c :192 - in function load_module(), the file is created to hold the sqlite database.  Pre-patch Asterisk will create the file 0660 instead of 0666 (left to the system administrator).  I am not particularly tied to this change; 0660 as a default for the sqlite database may make more sense.  I only change this file for consistency with the rest of Asterisk. By: Serge Vecher (serge-v) 2006-08-25 14:23:11 does this need an update to trunk here? By: Ben Klang (bklang) 2006-08-25 15:16:03 I can update it whenever necessary.  I would prefer to update only if it's ready to be merged so I don't need to do it repeatedly.  The last time I did update against svn trunk was in late June. By: Tilghman Lesher (tilghman) 2006-08-26 21:02:08 Yes, go ahead with the update to trunk.  We've already modified app_voicemail to use the 0777 and 0666 permissions. By: Ben Klang (bklang) 2006-09-02 15:07:23 Updated patch against rev 41810 is attached. By: Sergey Tamkovich (sergee) 2006-10-13 01:36:54 guys, what about mkstemp() call? this call doesn't respect UMASK, i think we should call chmod(filename, VOICEMAIL_FILE_MODE & ~UMASK) right after mkstemp()... By: jmls (jmls) 2006-11-12 02:47:33.000-0600 ping. housekeeping By: Ben Klang (bklang) 2006-11-12 09:55:20.000-0600 Housekeeping: Pong. Regarding mkstemp(), no I do not think a chmod is necessary.  temp files are not designed to be used outside Asterisk and the default behavior of mkstemp() is an intentional file creation mode of 0600 (since glibc 2.0.7). By: Tilghman Lesher (tilghman) 2006-11-12 11:15:14.000-0600 Yes, but voicemail .txt files are all initially created with mkstemp(), then moved to the appropriate INBOX directory upon successful conclusion of the recording.  Hence, they will all have permissions 0600, irrespective of the set umask or any ACLs. By: Ben Klang (bklang) 2006-11-12 11:27:00.000-0600 That sounds like a good candidate for a special case.  I will look into this and other mkstemp() calls. Last I talked to kpfleming, this patch was being considered for 1.4.  Is this still the case?  Anything I can do to help this along? By: Steve Murphy (murf) 2006-11-24 09:03:14.000-0600 I grepped around on the latest trunk stuff; and found the following; should these be updated as well? ./apps/app_sms.c:               int o = open (log_file, O_CREAT | O_APPEND | O_WRONLY, 0666); ./main/db1-ast/hash/hsearch.c:        dbp = (DB *)__hash_open(NULL, O_CREAT | O_RDWR, 0600, &info, 0); (this straggler will most likely not qualify;) ./contrib/scripts/vmail.cgi:    sysopen(RFILE, $rfile, O_WRONLY | O_CREAT | O_EXCL, 0666) or return -1; By: Steve Murphy (murf) 2006-12-20 18:38:48.000-0600 OK, no response here for a while; I am assuming that interest has been lost here. I have a simple choice: close this bug for lack of interest, and mark it as no changes necessary; or, I can simply merge the branch as it stands. What should I do? Going to leave it up to me? By: Ben Klang (bklang) 2006-12-20 18:56:49.000-0600 Hm, sorry about that.  I don't recall seeing an email for the last comment you left (or it gost lost in the SPAM). Anyway, here's my opinion: You wrote: >grepped around on the latest trunk stuff; and found >the following; should these be updated as well? > >./apps/app_sms.c: int o = open (log_file, O_CREAT | O_APPEND | O_WRONLY, 0666); Yes, this one should be modified accordingly. >./main/db1-ast/hash/hsearch.c: dbp = (DB *)__hash_open(NULL, O_CREAT | O_RDWR, 0600, &info, 0); No, this one was intentionally excluded.  Partially because it does not #include  asterisk.h (so the #define will be missing) and partially because it's really external to Asterisk, so I don't want to second-guess the authors of that module. (this straggler will most likely not qualify;) >./contrib/scripts/vmail.cgi: sysopen(RFILE, $rfile, O_WRONLY | O_CREAT | O_EXCL, 0666) or return -1; Right, can't reference the #define from perl.  Also, 0666 is what we're moving toward with this patch so no changes are required. By: Sergey Tamkovich (sergee) 2006-12-21 05:31:01.000-0600 I think that this patch should be merged into SVN-trunk. By: Steve Murphy (murf) 2006-12-21 14:19:21.000-0600 I merged these diffs into trunk via r.48767; 3 votes for (If you include mine), and 0 against. Motion was carried. Bug adjourned. By: Steve Murphy (murf) 2006-12-21 14:22:37.000-0600 I think this wraps this up. Merged into the trunk version. By: Sverre G (sverre) 2016-10-11 23:37:06.652-0500 I spent ages on the problem in which voicemail messages had permissions I didn't like and came across this thread once I started digging around in the voicemail.c code. What I discovered is that ~my_umask contains the umask that the asterisk process was started with (often 0644), and when joined with VOICEMAIL_FILE_MODE (0666) the result is the more restrictive mode (0644). The solution is to modify /usr/bin/safe_asterisk, specifically the line #UMASK=022, uncomment it and change it to UMASK=000 or UMASK=011. This will result in ~my_umask having the value 0777 or 0766, which when merged with 0666 results in 0666 and thus all voicemail files will have permissions of -rw-rw-rw which is much more useful (particularly if you have some third party web based system for listening to and deleting voicemails). I hope this helps someone so far down the track.