Feature Request: Add a new group for security rewritings and a rewriting for CALL TRANSACTION first

[franzreitmayer]

Add a new group "SECURITY" for security code rewritings, start with rewriting CALL TRANSACTION.

As of ABAP 7.4 CALL TRANSACTION which isn't followed by WITH AUTHORITY-CHECK or WITHOUT AUTHORITY-CHECK is obsolete and should not be used, but is still used in legacy code.
The rule should rewrite:
CALL TRANSACTION 'XD03'. CALL TRANSACTION 'XD03' using lt_param.
to
CALL TRANSACTION 'XD03' WITH AUTHORITY-CHECK. CALL TRANSACTION 'XD03' WITH AUTHORITY-CHECK USING lt_param.

Please see first poc implementation at https://github.com/franzreitmayer/abap-cleaner/blob/dev/com.sap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/security/SecurityCallTransactionWithAuth.java

main...franzreitmayer:abap-cleaner:dev

I'm interested in implementing this feature and send an according pull request.

...to be continued with further security rewritings... ;)

Thanks,
Franz

[fabianlupa]

I am a bit scared of this code formatter doing such functional (?) changes and them going unnoticed. If I understand the docs correctly adding WITH AUTHORITY-CHECK to a CALL TRANSACTION will result in the settings in TCDCOUPLES being ignored. Meaning some legacy applications relying on that feature will stop working or you need to retest them and adjust roles? Maybe I am misunderstanding the docs as I am not that familiar with the feature but currently this doesn't look like a compatible change to me.

https://help.sap.com/doc/abapdocu_757_index_htm/7.57/en-US/index.htm?file=abapcall_transaction_auth_obs.htm

Neither of the additions WITH|WITHOUT AUTHORITY-CHECK are specified in this variant of the statement CALL TRANSACTION. In this case, the content of the DDIC database table TCDCOUPLES specifies whether automatic authorization checks take place or not.

If the current transaction code and the called transaction code exist as a pair in the columns TCODE and CALLED of the database table TCDCOUPLES and if the database field OKFLAG of this row has the value X, the same authorization checks are performed as when the addition WITH AUTHORITY-CHECK is specified. If this is not the case, no check takes place.

[franzreitmayer]

That's a good point and you're of course right! This requires retesting and perhaps adjustment of authorizations. I think it's down to the nature of security fixes to change processing. Otherwise it would be pointless.
I just wonder wether it's possible/wanted to integrate this type of changes in abap-cleaner or not and how to do this. And I think we agree this is the first and basic question.

Personally I'm more scared of unintended authorizations rather than on a code cleaner fixing this and I think resulting code is still developers responsibility. I never percieved such tools as fire and forget.

Maybe this can be migtigated by defining options that must be considered to get it to work and hence draw the developers attention on this points?

Thanks and Regards,
Franz

[ConjuringCoffee]

I feel like the ABAP Cleaner shouldn't cause functional changes. In my opinion, adding WITH AUTHORITY-CHECK to CALL TRANSACTION fits better as an ATC check or quick-fix than in the ABAP Cleaner.

[fabianlupa]

Personally I'm more scared of unintended authorizations rather than on a code cleaner fixing this and I think resulting code is still developers responsibility. I never percieved such tools as fire and forget.

Personally this specific tool is fire and forget to me as it's basically advertised as a pretty printer on steroids or at least it acts like one being so fast and my muscle memory already presses ctrl + 4 in the basic rotation instead of shift + F1 by now.

I would be scared of being woken up in the middle of the night because some automated warehouses stopped working as they can no longer create deliveries from sales orders as the call transaction in the batch input processing suddenly stopped working. And all because of a transport labelled "Minor formatting changes". :P

Sure, you can make the rule optional and not part of any delivered profile but this being a security topic I would much rather want to block the transport with a centralized protected configuration based on the finding for changes (baseline) instead of relying on every developer having the correct config and no bad actors changing anything. Quickfixes can offer assisted fixes requiring the manual intervention you mention. Exemptions may also help in some use cases. Also the ATC check already exists.

[jmgrassau]

Hi franzreitmayer,

first of all, I'm impressed that you simply tried it out and apparently, successfully added a cleanup rule (or two of them)!

However, I really think from the way how ABAP cleaner is used (sometimes doing Ctrl+A, Ctrl+4 on veeery large code documents), it should be restricted to "fire and forget" changes, so after some initial configuration you can be sure even without looking that your code gets improved, while functionality is completely unchanged. At maximum, ABAP cleaner could add a TODO comment, as done by the rule "Delete unused variables" for variables that are assigned but never used – but especially for security relevant issues, an ATC check should be better for this.

So, I think I should find a suitable place in documentation on this repo to clarify this. Nevertheless, thanks for the discussion and hope you don't regret trying it out! And also thanks @fabianlupa for investigating the functional difference.

Kind regards,
Jörg-Michael

[franzreitmayer]

Hi Jörg,

Thanks for making this clear. I can understand this point and hence the feature request. One more question: Can you help me on track with ATC? I know how to implement own ATC checks, but I don't know how to write custom quickfixes, is that possible?

Thanks!
Franz

[jmgrassau]

Hi Franz,

I haven't implemented custom quickfixes yet, either, but did you come across ABAP Quick Fix? This project indeed has some functional overlap with ABAP cleaner, but the approach is different, because it uses quick fixes – so it's not doing "everything everywhere" in the "fire and forget" way, but allows you to do things in a more targeted, handpicked way (at the obvious cost of more user interaction). With this, ABAP Quick Fix can have functions like "Remove all comments", which you could never have in ABAP cleaner.

Anyway, I guess there you can get guidance and code samples of how quick fixes are implemented! (And if you do, please say hello to Łukasz Pęgiel :-)

Kind regards,
Jörg-Michael

[jmgrassau]

Hi Franz, Fabian and ConjuringCoffee,

thanks again for the fruitful discussion, this point was definitely worth clarifying! So, I'd add the following lines to the Limitations section of the README:

ABAP cleaner strives to offer cleanup rules that automatically detect what can be improved in the given ABAP code, so after some initial configuration, users shall be able to apply "everything everywhere" with just one keystroke. ABAP cleaner is therefore restricted to cleanup rules that keep functionality unchanged and can be applied without any additional user interaction, or need for subsequent inspection of changes.

Kind regards,
Jörg-Michael

[jmgrassau]

Hi Franz,

thanks again – the documentation was now enhanced with version 1.8.0!

Kind regards,
Jörg-Michael