CiviCRM Log File: Dates and Security

changed title from Log Dates to CiviCRM Log File: Dates and Security

added git:civicrm-core sig:security-improvement triaged labels

Those are really good points.

I would expect the date/time of the error to be consistent and machine parseable and the timezone explicit.

Definite +1. Recording with user-subjective-time just feels wrong. I'd wager that most humans and agents reading the log would expect a normalized time (e.g. per UTC or ini_get("date.timezone")).

I would expect the urls in the file to be XSS safe.

I don't follow. URLs aren't part of the log-format. The log-format allows open-ended strings which can include many different things (URLs, print_r(), XML tags, $e->getTraceAsTring(), etc).

The log-viewer-agent can be implemented in many different ways. I could see use-cases were log-viewer-agents need do:

header('Content-type: text/plain');
echo file_get_contents($log);

header('Content-type: text/html');
echo htmlentities(file_get_contents($log));

header('Content-type: text/html');
printf("<script>\nvar data = %s;\n</script>", json_encode(file_get_contents($log)));

header('Content-type: application/json');
echo json_encode(file_get_contents($log)));

This kind of encoding seems like the responsibility of the log-viewer-agent.

With that said, the log-format seems generally problematic. It would make sense if each log-entry were written as (say) a JSON object -- with more preserved structure -- so that it's easier for the log-viewer-agent to do nice things with the data.

re: xss safety.

Yes to log-viewer-agent being responsible for safety of what it's displaying (and now implemented in the case of the log viewer extension).

On the other hand, I was imagining looking at that log file in other contexts, e.g. vi on the server, where a dangerous link was included in the log entry.

Agreed that for example an apache log file has the same issue, so am fine with removing this aspect of the issue if no one else can think of a way that this log file might create security concerns.

I do notice that the drupal watchdog sanitizes its arguments (which is good, since the watchdog is normally accessed via the web interface), and generally is more structured that the civicrm error log, in support of your last paragraph.

re: a PR, I confess to being baffled by where the current code lives. The actual "log" function of the log object seems to be doing the work, but that log object is getting created with:

return Log::singleton('file', \Civi::$statics[CLASS]['logger_file' . $prefix], '');

and I have failed to figure out which of the several Log.php files that might referring to (if any).

Presumably this complexity comes down to being able to log things early on in the bootstrap process.

I agree that log files should be able to contain all manner of crap you can't trust to click on: the viewer should make it safe, and/or the consumer should be careful!
I agree a standard date format is important. I also have a lot of other opinions on logs - see https://lab.civicrm.org/extensions/taggedlog

Whoa, it looks like you've already solved this problem! Maybe I just need to extend my logviewer to handle your log format!

I like my extension quite a lot, though it's still a hacky hybrid: I might prefer it if each log entry was actually a JSON object, but then you need jq or such to query it really.

More relevantly: one problem I've found since running that extension is that some code writes direct to the log file, or at least without doing Civi::log()->facility(...), e.g. calls CRM_Core_Error::debug() directly.

I was curious how your extension intercepts logging calls, I haven't been keeping up on all the ways that you can put stuff into that log file. Can you summarize where your code does the magic? I was assuming the first step would be to put some kind of layer of abstraction in front of the code that was writing the date in that terrible format and allow extensions to extend it. I'm not convinced that we need the ability to change the date format (and where in the code is that hapenning anyway?), but having a more machine parseable format for each entry is hard to argue with.

@AlanDixon

In the hook_civicrm_container is where I replace the log manager service: https://lab.civicrm.org/extensions/taggedlog/-/blob/main/taggedlog.php#L10

with my own: https://lab.civicrm.org/extensions/taggedlog/-/blob/main/Civi/TaggedLog/Manager.php

which will ensure the log class that gets instantiated is also my custom one: https://lab.civicrm.org/extensions/taggedlog/-/blob/main/Civi/TaggedLog/Manager.php#L19

A pull request related to this issue has been opened with the title: #4270 (closed) createDebugLogger/ConfigAndLog: use a standard date format

added has-pull-request label

Hi,

Thank you for raising an issue to help improve CiviCRM. As you may know, this issue has not had any activity for quite some time, so we have closed it.

We would like you to help us to determine if this issue should be re-opened:

If this issue was reporting a bug, can you attempt to reproduce it on a latest version of CiviCRM?
If this issue was proposing a new feature, can you verify if the feature proposal is still relevant? Did it get the concept-approved label? Have other people also shown interest? Could it be implemented as an extension?

If the answer to either question is yes, please feel free to comment or re-open the issue. Please also consider:

Is it something that you could help implement, either by sending a patch or hiring someone who can?
Would you be willing to fund the work, either through a CiviCRM partner, or the Core Team?

Thank you for your help and contributions to CiviCRM.

P.S. This is an automated message, see infra/gitlab issue 20. We understand that automatic responses are annoying, but given the number of open issues as the project evolves, we need a bit of help to triage and prioritise the most relevant issues.

closed

Thanks support_bot_dev, I'm happy to see this issue closed.

And thanks to matheiu for the work of actually fixing things.

Note that only the date format w/ tz has implemented, any issue of doing something fancy with the actual log entry has been ignored/dropped.

CiviCRM Log File: Dates and Security

Overview

Expected behaviour

Comments

Child items 0

Activity